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

[Peter Collingbourne via Phabricator via llvm-commits]

pcc added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64ISelLowering.cpp:10642
+  }
+  uint64_t Offset = MinOffset + GN->getOffset();
+
----------------
t.p.northover wrote:
> 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.
Yes, that is exactly what happens. I will leave a comment here.


================
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
----------------
t.p.northover wrote:
> Is this some object format constraint? Because in straight LLVM IR I don't think there's any rule against going out of bounds.
It is a code model constraint. Imagine that you have an executable with virtual size of exactly 2GB (which just fits into the small code model), a function at the very start and a variable at the very end. If the function takes the address of the variable and adds a large out-of-bounds offset, folding the offset would push the address outside of the range allowed by the code model, and the linker would be unable to relocate the ADRP instruction. We would still be able to compute the address of the start of the variable and add the offset in code.


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


https://reviews.llvm.org/D45199