[PATCH] D45199: AArch64: Allow offsets to be folded into addresses with ELF.

[Tim Northover via Phabricator via llvm-commits]

t.p.northover added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10642
+  }
+  uint64_t Offset = MinOffset + GN->getOffset();
+
----------------
I'm not quite sure about the signs here. I **think** your code ends up treating negative offsets as huge positive ones, which is equivalent to ignoring them (as it does with ISD::SUB uses). 

That's OK since I expect it's a rare case, but I'd be a bit happier if there was a comment about how it works.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10644-10645
+
+  // Check whether folding this offset is legal. It must not go out of bounds of
+  // the referenced object to avoid violating the code model, and must be
+  // smaller than 2^21 because this is the largest offset expressible in all
----------------
Is this some object format constraint? Because in straight LLVM IR I don't think there's any rule against going out of bounds.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10660
+  return DAG.getNode(ISD::SUB, DL, MVT::i64, Result,
+                     DAG.getConstant(Offset, DL, MVT::i64));
+}
----------------
Shouldn't this be `MinOffset`? It looks like this will always be `GV+0`, when we started with `GV+GN->getOffset()`


https://reviews.llvm.org/D45199