[PATCH] D87510: [SystemZ] Don't emit PC-relative memory accesses to unaligned (packed) symbols.

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp:1496
+  // The alignment of the symbol itself must be at least the store size.
+  const GlobalValue *GV = GA->getGlobal();
+  const DataLayout &DL = GV->getParent()->getDataLayout();
----------------
You check for the GA == nullptr case above, don't we need to check this here as well?


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp:1502
+  return true;
+}
+
----------------
Maybe the whole thing would be a bit clearer if you grouped the tests by category, e.g. first to all tests on the MemAccess itself, followed by tests on the MMO, followed by tests on the GA/GV?  That might also eliminate duplicate nullptr checks.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp:1472
+
+  if (!SystemZISD::isPCREL(BasePtr.getOpcode()) ||
+      !MemAccess->getOffset().isUndef()) // potential index register
----------------
jonpa wrote:
> uweigand wrote:
> > I don't think we need to check PCREL here, that should already be handled by the pattern (and has nothing to do with alignment, really).
> I see it as needed because TableGen emits the code in SystemZGenDAGISel.inc in such a way that storeLoadIsAligned() is called before selectPCRelAddress(). That means that a lot of different addresses other than PCREL will show up, which needs to be filtered away.
> 
I still don't understand why this "needs to be filtered away" here.    Why can't this routine simply return true/false depending on whether the argument is aligned, no matter what operand it gets?


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp:1476
+
+  // An access to GOT or constant pool is always aligned.
+  if (const PseudoSourceValue *PSV =
----------------
jonpa wrote:
> uweigand wrote:
> > Well, there could still be an unaligned offset.  I think this check needs to be done after the offset check.
> It seems that a load from CP is not a GlobalAddress, but there should still the possibility of an immediate offset reflected in the MMO. I added a check for this.
> 
> It seems safest to check both these offsets independently when they are present.
> 
OK.


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

https://reviews.llvm.org/D87510