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

[Jonas Paulsson via Phabricator via llvm-commits]

jonpa added inline comments.


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp:1502
+  return true;
+}
+
----------------
uweigand wrote:
> 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.
OK, I'll try that


================
Comment at: llvm/lib/Target/SystemZ/SystemZISelDAGToDAG.cpp:1472
+
+  if (!SystemZISD::isPCREL(BasePtr.getOpcode()) ||
+      !MemAccess->getOffset().isUndef()) // potential index register
----------------
uweigand wrote:
> 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?
My idea was to keep it simple by making a function for just the PCRel loads/stores since they are the only ones that have the alignment requirement.

The PCREL_ nodes make it natural to treat these accesses separately from other accesses.

I suppose returning "false" if it is not a PCREL_ node is not very logical, so I can try to avoid this check instead...


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

https://reviews.llvm.org/D87510