This is an archive of the discontinued LLVM Phabricator instance.

[GlobalOpt] Do not shrink global to bool for an unfavorable AS
ClosedPublic

Authored by cdevadas on Sep 15 2021, 5:31 AM.

Details

Summary

Do not call TryToShrinkGlobalToBoolean for address spaces
that don't allow initializers. It inserts an initializer value
while shrinking to bool. Used the target hook introduced with
D109337 to skip this call for the restricted address spaces.

Diff Detail

Event Timeline

cdevadas created this revision.Sep 15 2021, 5:31 AM
cdevadas requested review of this revision.Sep 15 2021, 5:31 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2021, 5:31 AM
arsenm added inline comments.Sep 15 2021, 6:05 AM
llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll
1–2

Needs REQUIRES: amdgpu-registered-target, or to move to an AMDGPU test directory

cdevadas updated this revision to Diff 372693.Sep 15 2021, 6:27 AM

Added REQUIRES field in the lit test.

jdoerfert added inline comments.Sep 15 2021, 7:19 AM
llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll
15

Why is the check and amdgcn line the same, that I don't understand. Should it not trigger for "check"?

arsenm added inline comments.Sep 15 2021, 7:24 AM
llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll
15

The host line won't fail the address space check, and it serves as a sanity check that the transform would be performed otherwise

arsenm added inline comments.Sep 15 2021, 7:32 AM
llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll
2

Probably should use an explicit target for the reference line

cdevadas added inline comments.Sep 15 2021, 9:01 AM
llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll
2

I intended to capture the default behavior along with the output for AMDGPU.
The default implementation of canHaveNonUndefGlobalInitializerInAddressSpace allows only AS(0) to have an initializer.
There is no target that currently allows initializer for AS(3) and any arbitrary target used here will transform the code similar to AMDGPU.
I will use x86. The test in the future shouldn't break without one.

tra added a comment.Sep 15 2021, 9:43 AM

The patch looks reasonable, but I have a question about the test.

llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll
34–42

I'm confused. I see no differences between AMDGCN and CHECK anywhere in the test file. AFAICT, the test would work if I were to use CHECK in the test with --mtriple=amdgcn-amd-amdhsa. If both CHECK and AMDGCN are identical, why do we need two different check patterns?

Did I miss something?

jdoerfert added inline comments.Sep 15 2021, 9:52 AM
llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll
34–42
cdevadas updated this revision to Diff 372751.Sep 15 2021, 10:56 AM

Fixed the lit test to have only one check pattern.

arsenm added inline comments.Sep 15 2021, 7:09 PM
llvm/test/Transforms/GlobalOpt/shrink-global-to-bool.ll
3

If they're going to have the same output, there's no point in the second run line. The cpu run line was only useful as a reference to make sure the transform happened otherwise

5

Would also need x86 check

cdevadas updated this revision to Diff 372857.Sep 15 2021, 7:55 PM

Retained just one RUN line.

tra accepted this revision.Sep 16 2021, 9:39 AM
This revision is now accepted and ready to land.Sep 16 2021, 9:39 AM