[PATCH] D51030: [AArch64] Optimise load(adr address) to ldr address

[Dave Green via Phabricator via llvm-commits]

dmgreen added inline comments.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:1896
+def alignedload : PatFrag<(ops node:$ptr), (load node:$ptr), [{
+  return cast<LoadSDNode>(N)->getAlignment() >= 4;
+}]>;
----------------
efriedma wrote:
> dmgreen wrote:
> > efriedma wrote:
> > > Checking the alignment of the load isn't going to do the right thing in general; you need to check that the global is aligned.  (Normally, a misaligned load is UB, but this would turn it into a link error.)
> > OK thanks. Can you give more details on the difference? Is the load's alignment wrong or inefficient? Or do you mean that in the event that load's alignment is wrong in a way that is UB, it needs to not be link error?
> > 
> > I would expect if we had an aligned global, <something> (instsimplify maybe?) would set the alignment on the load to that of the global. And we are using the alignment for a ldr here, so it coming from a load at least makes sense in that way.
> > 
> > There are other examples of aligned load patterns like this, such as alignedload16 from arm or alignedload from x86. They may not be expecting to be acting on the same global data though.
> > Or do you mean that in the event that load's alignment is wrong in a way that is UB, it needs to not be link error?
> 
> I mean this: we can't reliably prevent optimizations that clone code, like inlining, from introducing UB, so UB can't be a link error.
> 
> The requirement here is different from alignedload because normally aligned loads only fault (or load incorrect data) if you violate the alignment requirement; the code still links.  For example, on x86, "movapd 1, %xmm0" is a valid instruction, even though it will always fault.
I see.

We currently use other ldr literal relocations, but I guess those are mostly to gotslots so cant be unaligned in the same way. I'm not sure about the tls ones, I think they all go through a got too.


================
Comment at: lib/Target/AArch64/AArch64InstrInfo.td:1900
+    if (auto *GV = dyn_cast<GlobalVariable>(G->getGlobal()))
+      Align = MF->getDataLayout().getPreferredAlignment(GV);
+    return Align >= 4 && G->getOffset() % 4 == 0;
----------------
efriedma wrote:
> I'd suggest just using `unsigned Align = G->getGlobal()->getPointerAlignment()` here, which should reliably do the right thing for all GlobalObjects.
> 
> Should be possible to match a global address node directly using something like the following:
> 
> ```
> def alignedglobaladdr : PatLeaf<(tglobaladdr), [{
>   auto *G = cast<GlobalAddressSDNode>(N);
>   // etc.
> }]>;
> ```
Thanks for the suggestions. Will do.

I was looking for a way to avoid these multiple Pat's, having a PatLeaf that matched either tglobaladdr or tconstpool. You can let me know if it's worth it over multiple patterns.


https://reviews.llvm.org/D51030