[PATCH] D80370: [CodeGen] Ensure callers of CreateStackTemporary use sensible alignments

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:893
+  SDValue StackPtr =
+      DAG.CreateStackTemporary(Op.getValueType().getStoreSize(), Align);
   // Emit a store to the stack slot.
----------------
david-arm wrote:
> efriedma wrote:
> > You changed the alignment of the stack temporary... but you still didn't change the alignment of the load or the store to that stack slot, so they're still wrong.  (Same applies to other changes.)
> OK fair enough, but previously the code also didn't specify the same alignment chosen by CreateStackTemporary either. So I didn't realise I had to explicitly add the alignment to the loads/stores. My new NEON test now passes with an alignment of 16 bytes, whereas previously it was 32. Since it just worked I assumed that the StackPtr itself contained the alignment, but I can certainly take a look. I don't mind adding the alignments explicitly to the loads and stores at all if that's needed.
When getStore() etc. is called without an explicit alignment, the alignment is inferred based on VT of the stored value, not the pointer.

If CreateStackTemporary() also computes the alignment based on the VT, it's fine: the resulting alignment is consistent.  But if CreateStackTemporary() does something different, then it's inconsistent, and we might miscompile.

We're gradually moving towards deprecating the getStore() variants that don't explicitly specify the alignment because this is confusing.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:1474
   Store = DAG.getTruncStore(Store, dl, Elt, EltPtr,
                             MachinePointerInfo::getUnknownStack(MF), EltVT);
 
----------------
It would be a good idea to explicitly specify the alignment to the getTruncStore, too.  Probably it will be naturally aligned anyway, but better to be safe.  Something like `commonAlignment(SmallestAlign, EltVT.getSizeInBits() / 8)`.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2253
       ISD::EXTLOAD, dl, N->getValueType(0), Store, StackPtr,
       MachinePointerInfo::getUnknownStack(DAG.getMachineFunction()), EltVT);
 }
----------------
See above comment about getTruncStore.


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

https://reviews.llvm.org/D80370