Page MenuHomePhabricator

[AMDGPU] Add Relocation Constant Support
ClosedPublic

Authored by csyonghe on Mar 19 2020, 11:43 AM.

Details

Summary

This change adds amdgcn.reloc.constant intrinsic to the amdgpu backend, which will compile into a relocation entry in the resulting elf.

The intrinsics takes a MetadataNode (String) as its only argument, which specifies the symbol name of the relocation entry.

SelectionDAGBuilder::getValueImpl is changed to allow metadata operands passed through to ISel.

Diff Detail

Event Timeline

csyonghe created this revision.Mar 19 2020, 11:43 AM
csyonghe updated this revision to Diff 251514.Mar 19 2020, 5:33 PM
csyonghe set the repository for this revision to rG LLVM Github Monorepo.Mar 20 2020, 11:09 AM

It seems weird to me to expose relocation names so directly. Why isn't this just exposing whatever specific construct you need?

llvm/test/CodeGen/AMDGPU/amdgpu-reloc-const.ll
19

You can just use 0.0

This is suggested by @nhaehnle
I like this idea because it does not lock into any specific data structure prematurely in amdgcn, which the backend really does not care. In the end you want to control the name of the symbol exported in the elf.
This will enable us to use the intrinsic for many different purposes in llpc, and push the responsibility of naming a relocation symbol to llpc instead of in amdgpu.

Thank you for doing this. And yes, the point of the generic mechanism is to decouple the frontend and backend a bit more, as the envisioned use case for this are very graphics-specific, and we will want to evolve it on the frontend side without having to juggle LLVM changes for each one individually.

Please augment the test with a run line that checks the ELF relocation entries as well? I.e. parsing the generated ELF (-filetype=obj) with llvm-readobj. There are existing tests in CodeGen/AMDGPU which do this that you can imitate.

llvm/include/llvm/IR/IntrinsicsAMDGPU.td
1990–1991

The comment is out of date.

llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1764–1777

Please either move this hunk into a separate patch or add a test case to this one that shows the effect.

Also, the ZERO_EXTEND handling still looks like it'd need proper type checks.

1842–1843

This should be unnecessary.

csyonghe updated this revision to Diff 252460.Mar 24 2020, 5:15 PM
csyonghe marked 4 inline comments as done.

Revised per @nhaehnle's comments.

csyonghe marked an inline comment as not done.Mar 24 2020, 5:16 PM
csyonghe added inline comments.
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1764–1777

This change is necessary for correct selection of the offset-from-a-register version of the load instruction following a offset relocation. Otherwise a lot of unnecessary code will get generated.
Since this is in SelectSMRDOffset, the only type that can appear in the ZERO_EXTEND is int32, no? What kind of type check do you think is necessary here?

1842–1843

I want to handle the case where Addr is in the form of base+offset, where offset is not a constant. In this case, the register-offset version of the load instruction should be selected (see line 1765). Without this, that logic does not trigger.

nhaehnle added inline comments.Mar 26 2020, 12:07 PM
llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp
1764–1777

Okay, thank you for the explanation and the added test. However, relying on it being i32 is unsafe here. It's conceivable that there could be a zeroext from i16, for example. (Now in practice, that gets combined to an anyext followed by and today, but why wouldn't that potentially change at some point in the future?)

Simply checking that the operand of the ZERO_EXTEND is an i32 should be fine, I don't think you have to try to handle anything more general than that.

1842–1843

Ah of course. Thanks for the explanation.

csyonghe updated this revision to Diff 252972.Mar 26 2020, 1:51 PM

Added operand type check for ZERO_EXT.

csyonghe updated this revision to Diff 252988.Mar 26 2020, 3:00 PM
csyonghe updated this revision to Diff 252994.Mar 26 2020, 3:19 PM
csyonghe marked an inline comment as done.Mar 26 2020, 4:22 PM
nhaehnle accepted this revision.Mar 27 2020, 5:34 AM

Thanks, LGTM

This revision is now accepted and ready to land.Mar 27 2020, 5:34 AM
csyonghe marked 4 inline comments as done.Mar 27 2020, 10:20 AM
This revision was automatically updated to reflect the committed changes.