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

[Yong He via Phabricator via llvm-commits]

csyonghe marked an inline comment as not done.
csyonghe added inline comments.


================
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;
+    }
----------------
nhaehnle wrote:
> 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.
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?



================
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);
----------------
nhaehnle wrote:
> This should be unnecessary.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76440