Page MenuHomePhabricator

[AMDGPU] Lower null pointers in static variable initializer
ClosedPublic

Authored by yaxunl on Jan 30 2017, 8:25 AM.

Details

Summary

For amdgcn target Clang generates addrspacecast to represent null pointers in private and local address spaces.

In LLVM codegen, the static variable initializer is lowered by virtual function AsmPrinter::lowerConstant which is target generic. Since addrspacecast is target specific, AsmPrinter::lowerConstant is unable to lower addrspacecast in the null pointer representation.

This patch overrides AsmPrinter::lowerConstant with AMDGPUAsmPrinter::lowerConstant, which is able to lower the target-specific addrspacecast in the null pointer representation so that -1 is correctly emitted in codeobject as initial value for null pointers.

Diff Detail

Event Timeline

yaxunl created this revision.Jan 30 2017, 8:25 AM
arsenm added inline comments.Jan 31 2017, 10:45 AM
lib/Target/AMDGPU/AMDGPUMCInstLower.cpp
162–168

We should factor the constant folding of pointers into a single place. The same logic is already repeated in SIISelLowering and I have a need of it in another place.

Maybe we should put a constant fold pointer helper in AMDGPUTargetMachine?

yaxunl updated this revision to Diff 86648.Feb 1 2017, 9:21 AM

Refactored constant folding of non-zero null pointer to a separate file.

arsenm added inline comments.Feb 1 2017, 9:26 AM
lib/Target/AMDGPU/Utils/AMDGPUConstantFoldUtils.cpp
17 ↗(On Diff #86648)

This doesn't need to be its own file and isn't the right API to be re-used. I think the right place is in AMDGPUTargetMachine because this is logically part of the DataLayout.

The base function should just be the address spaces so it can be used in the DAG and MI

arsenm edited edge metadata.Feb 1 2017, 9:27 AM

Better test name might be nullptr-constant-initializer

test/CodeGen/AMDGPU/nullptr.ll
12

Should include tests for the other address spaces too

yaxunl added inline comments.Feb 1 2017, 10:07 AM
lib/Target/AMDGPU/Utils/AMDGPUConstantFoldUtils.cpp
17 ↗(On Diff #86648)

I took a look of TargetMachine and its derived classes, I did not see a precedence to put such information there. Is it the right place to hold such information?

Another question. I am not quite sure about the desired API for this function. Do you want something like this?

uint64_t getNullPointerValue(unsigned AddrSpace);

which returns -1 for private/local address spaces and 0 for other address spaces?

arsenm added inline comments.Feb 1 2017, 10:58 AM
lib/Target/AMDGPU/Utils/AMDGPUConstantFoldUtils.cpp
17 ↗(On Diff #86648)

I think so, it's more datalayout/constant related. e.g. getNameWithPrefix.

I think some of this handling is incorrectly in TargetLowering, e.g. D20758.

Something like that seems right

yaxunl updated this revision to Diff 86824.Feb 2 2017, 8:50 AM

Added getNullPtrValue to TargetMachine and more tests.

yaxunl updated this revision to Diff 86886.Feb 2 2017, 3:07 PM

Keep getNullPointerValue to AMDGPUTargetMachine only.

arsenm accepted this revision.Feb 2 2017, 3:28 PM

LGTM except for minor detail

lib/Target/AMDGPU/AMDGPUTargetMachine.cpp
225–233

Body of the function should go in the header

228

Might as well handle REGION as well

This revision is now accepted and ready to land.Feb 2 2017, 3:28 PM
yaxunl added a comment.Feb 3 2017, 7:49 AM

Thanks. Will fix it when submitting.

yaxunl closed this revision.Feb 10 2017, 1:58 PM