This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Use getConstantStringInfo for printf format strings
ClosedPublic

Authored by arsenm on Jan 6 2023, 5:03 PM.

Details

Reviewers
sameerds
vikramRH
yaxunl
Group Reviewers
Restricted Project
Summary

Tolerated printf format strings that are indexed globals and fixes
asserting on non-null terminated strings.

Diff Detail

Event Timeline

arsenm created this revision.Jan 6 2023, 5:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 5:03 PM
arsenm requested review of this revision.Jan 6 2023, 5:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 6 2023, 5:03 PM
Herald added a subscriber: wdng. · View Herald Transcript
vikramRH added a comment.EditedJan 9 2023, 1:29 AM

I would like to see this and other printf related patches land and I want to take the new changes as base to update D138702 for now due to issue priority.

The change itself looks straightforward to me, but I am struggling to understand what is happening in the tests. @yaxunl , can you please comment?

llvm/test/CodeGen/AMDGPU/opencl-printf.ll
549

I can't make out from the spec if OpenCL allows a format string with no terminator. Is this UB? Is it supposed to produce a diagnostic?

605

Same question. UB? Diagnostic?

625

Are the previous values plain wrong? Can this be reproduced in an actual OpenCL kernel? Will it crash the kernel?

arsenm added inline comments.Jan 10 2023, 10:44 AM
llvm/test/CodeGen/AMDGPU/opencl-printf.ll
549

If you have a string literal in C it ends up being a null terminator. This covers not crashing on valid IR that opencl just happens to never produce

605

We can handle it just fine, no need to diagnose

625

These are just the indexes into the list of format string metadata, these will get renumbered every time a new function is inserted into this file

sameerds accepted this revision.Jan 12 2023, 6:46 AM
This revision is now accepted and ready to land.Jan 12 2023, 6:46 AM