Skip to content

Don't split printf if %s is static #960

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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.

@@ -380,9 +380,11 @@ PreservedAnalyses HipPrintfToOpenCLPrintfPass::run(Module &Mod,
// 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.

while (FormatSpecCount--) {
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