[PATCH] D76440: [AMDGPU] Add Relocation Constant Support

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added a comment.

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.



================
Comment at: llvm/include/llvm/IR/IntrinsicsAMDGPU.td:1990-1991
+
+// Emits the offset of the descriptor for the given set and binding.
+// Will generate a fixup.
+def int_amdgcn_reloc_constant : Intrinsic<
----------------
The comment is out of date.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1764-1777
+  if (!C) {
+    if (ByteOffsetNode.getValueType().isScalarInteger() &&
+        ByteOffsetNode.getValueType().getSizeInBits() == 32) {
+      Offset = ByteOffsetNode;
+      Imm = false;
+      return true;
+    }
----------------
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.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1842-1843
        Addr->getFlags().hasNoUnsignedWrap()) &&
-      CurDAG->isBaseWithConstantOffset(Addr)) {
+      (CurDAG->isBaseWithConstantOffset(Addr) ||
+       Addr.getOpcode() == ISD::ADD)) {
     SDValue N0 = Addr.getOperand(0);
----------------
This should be unnecessary.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D76440/new/

https://reviews.llvm.org/D76440