This is an archive of the discontinued LLVM Phabricator instance.

[clang] Preserve names of addrspacecast'ed values.
ClosedPublic

Authored by vzakhari on Jun 26 2019, 3:11 PM.

Details

Summary

Attach ".ascast" suffix to a value name when generating addrspacecast for it. This improves IR readability, e.g. for alloca variables, since all users of the variable will be using the addrspacecast value instead of the original alloca.

Diff Detail

Repository
rC Clang

Event Timeline

vzakhari created this revision.Jun 26 2019, 3:11 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 26 2019, 3:11 PM

Adding more reviewers.

The change itself doesn't really bother me at all, however the test will fail if we build without preserving names. In order to validate this you'll have to do fno-discard-value-names on a test.

clang/test/CodeGenOpenCL/address-spaces-conversions.cl
16

You probably don't want to remove the wildcard here. If this is built without names being saved, this change will fail.

Please don't check IR names in test output. That actually includes anonymous names like %2; these should always be tested with FileCheck variables. I suggest using %.* as the pattern; if you're matching the LHS of an LLVM assignment, that shouldn't have problems with accidentally matching too much.

vzakhari marked an inline comment as done.Jul 8 2019, 1:20 PM

Please don't check IR names in test output. That actually includes anonymous names like %2; these should always be tested with FileCheck variables. I suggest using %.* as the pattern; if you're matching the LHS of an LLVM assignment, that shouldn't have problems with accidentally matching too much.

I agree that checking IR names is a bad practice. I can change all these tests so that they either use FileCheck variables or -fno-discard-value-names (e.g. for clang/test/CodeGenOpenCLCXX/addrspace-of-this.cl, which relies on the names for checking) - would that be an appropriate solution?

clang/test/CodeGenOpenCL/address-spaces-conversions.cl
16

In the tests that already use variable names I stick to checking the names, e.g. this test already assumes that the names are preserved (%var_priv in this check), so I am just checking that 'ascast' suffix is added. I guess I will have to add -fno-discard-value-names for this test - do you agree?

I don't know what I think about widespread use of -fno-discard-value-names for now; please continue to use FileCheck variables, and we can make a holistic decision about that flag later.

I don't know what I think about widespread use of -fno-discard-value-names for now; please continue to use FileCheck variables, and we can make a holistic decision about that flag later.

Sorry, I have one particular question about clang/test/CodeGenOpenCLCXX/addrspace-of-this.cl:
Test the address space of 'this' when invoking copy-constructor.
COMMON: [[C1GEN:%c1.ascast[0-9]*]] = addrspacecast %class.C* %c1 to %class.C addrspace(4)*

This check seems to rely on %c1 name already. I guess the matching may go off, if we do not use actual names on the right hand side of the assignment. Should I do anything about the right hand side, or just use a generic wildcard on the left hand side?

I don't know what I think about widespread use of -fno-discard-value-names for now; please continue to use FileCheck variables, and we can make a holistic decision about that flag later.

Sorry, I have one particular question about clang/test/CodeGenOpenCLCXX/addrspace-of-this.cl:
Test the address space of 'this' when invoking copy-constructor.
COMMON: [[C1GEN:%c1.ascast[0-9]*]] = addrspacecast %class.C* %c1 to %class.C addrspace(4)*

This check seems to rely on %c1 name already. I guess the matching may go off, if we do not use actual names on the right hand side of the assignment. Should I do anything about the right hand side, or just use a generic wildcard on the left hand side?

I forgot that of course you're updating this test already. If you can update the test to use variables without damaging the quality of the test, please do so.

vzakhari updated this revision to Diff 208564.Jul 8 2019, 6:14 PM

I changed the tests to use FileCheck variables whenever possible.

rjmccall accepted this revision.Jul 8 2019, 8:17 PM

Thanks!

This revision is now accepted and ready to land.Jul 8 2019, 8:17 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 10 2019, 10:10 AM