Skip to content

Conversation

@lights0123
Copy link

This is part of my work on HipScript, where I use this project to run HIP code in the browser with WebGPU.

Using printf with the %s specifier, like in __assert_fail, causes chipStar to split the call into several. For example:

printf("File %s errored\n", "main.hip");
// turns into
printf("File ");
printf("main.hip");
printf(" errored\n");

This causes problems if multiple threads are printing, since these calls can interleave, producing output like:

File File main.hipmain.hip errored
errored

This PR will keep %s specifiers, but only if the argument can be converted to a global string. In my case, where I compile to Vulkan afterwards with Clspv, this allows the compiler to convert the string pointer to an integer ID.

Copy link
Collaborator

@pjaaskel pjaaskel left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks. The improvement makes sense, but please check my feedback.

// Skip the original format string arg and recreate it.
OrigCallArgI++;

std::string toAdd;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
std::string toAdd;
std::string ToAdd;

Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also pls add a comment what is this variable for.

assert(OrigCallArgI != OrigCallArgs.end());
Value *OrigArg = *OrigCallArgI++;

if (FPExtInst *fpext = dyn_cast<FPExtInst>(OrigArg)) {
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems separate from the feature described. Please add a comment what is this code block for.

@pvelesko
Copy link
Collaborator

pvelesko commented Jan 8, 2025

printf unit tests failing fyi

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants