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

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

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


================
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);
----------------
csyonghe wrote:
> 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.
Ah of course. Thanks for the explanation.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D76440