This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Run internalize symbols at -O0
ClosedPublic

Authored by arsenm on Sep 14 2017, 8:13 PM.

Details

Reviewers
rampitec
kzhuravl
Summary

The relocations used for externally visible functions
aren't supported.

I'm not sure if it would be better to actually try
supporting the relocations.

Diff Detail

Event Timeline

arsenm created this revision.Sep 14 2017, 8:13 PM
t-tye added a comment.Sep 14 2017, 9:02 PM

AFAIK the relocations are supported but you need to be using the PLT so the relocations are against the data section and not the text section.

rampitec added inline comments.Sep 15 2017, 11:05 AM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
330

Looks like there is now isEntryFunctionCC() which can be used instead of this switch.

arsenm updated this revision to Diff 115680.Sep 18 2017, 10:49 AM

Use isEntryFunctionCC

rampitec added inline comments.Sep 18 2017, 11:23 AM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
326

Looks like it changes original logic. All functions were internalized except for declarations and kernels.

arsenm added inline comments.Sep 18 2017, 2:20 PM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
326

It's the same, also no tests fail

rampitec added inline comments.Sep 18 2017, 2:30 PM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
326

If a function is called and not declaration it used to be internalized, with this change it will not.

arsenm added inline comments.Sep 18 2017, 2:40 PM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
326

Yes it is. This is the reason for doing this and is the func_used test I added

rampitec added inline comments.Sep 18 2017, 2:43 PM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
326

Then what happens if function is finally inlined and no calls left? I suppose it will remain in the object, which is not we really want.

arsenm added inline comments.Sep 18 2017, 2:56 PM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
326

There is no change here in behavior from what was here before other than also internalizing at -O0. Regardless of whether the function is used or not it is internalized

rampitec added inline comments.Sep 18 2017, 3:01 PM
lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
326

There is change in behavior with the optimizations. You have removed this line to indicate this change:

CHECK-NOT: foo_used

With optimizations foo_used shall not be in the output.

test/CodeGen/AMDGPU/internalize.ll
26

I have check prefixes ALL, OPT and OPTNONE. The check prefix CHECK does not exist.

arsenm updated this revision to Diff 115742.Sep 18 2017, 4:09 PM

Fix check prefixes, directly return logical expression

rampitec added inline comments.Sep 18 2017, 4:16 PM
test/CodeGen/AMDGPU/internalize.ll
26

Does it work? Code now seems to be right, but this check shall fail. foo_used is always_inline, it shall be internalized, inlined and eliminated.

arsenm added inline comments.Sep 18 2017, 4:40 PM
test/CodeGen/AMDGPU/internalize.ll
26

This test is actually broken as is. The calling convention for amdgpu_kernel doesn't match the call site calling convention, so this gets turned into trap/unreachable which is why it gets eliminated.

26

We probably shouldn't allow calls to amdgpu_kernel for the IR calling convention.

rampitec added inline comments.Sep 18 2017, 4:43 PM
test/CodeGen/AMDGPU/internalize.ll
26

Ouch. This is mistake in the test, it shall be just a function.

arsenm updated this revision to Diff 115753.Sep 18 2017, 4:52 PM

Remove broken test

rampitec edited edge metadata.Sep 18 2017, 4:58 PM

Can you please restore foo_used and just fix calling convention? This case is not covered now.

test/CodeGen/AMDGPU/internalize.ll
34

Typo in func name?

Can you please restore foo_used and just fix calling convention? This case is not covered now.

That's the new func_used I added, which is the same thing.

arsenm updated this revision to Diff 115757.Sep 18 2017, 5:03 PM

Fix names

Can you please restore foo_used and just fix calling convention? This case is not covered now.

That's the new func_used I added, which is the same thing.

Not really, it is noinline. The old one was always inline with the opposite check condition.

arsenm updated this revision to Diff 115758.Sep 18 2017, 5:15 PM

Add different inline attribute functions

rampitec added inline comments.Sep 18 2017, 5:19 PM
test/CodeGen/AMDGPU/internalize.ll
35

OPT-NOT, not OPT-NONE.

arsenm updated this revision to Diff 115771.Sep 18 2017, 6:21 PM

Fix check not

This revision is now accepted and ready to land.Sep 18 2017, 10:23 PM
arsenm closed this revision.Sep 19 2017, 12:41 AM

r313616