[PATCH] D140638: [Codegen][LegalizeIntegerTypes] New legalization strategy for scalar shifts: shift through stack

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri marked 2 inline comments as done.
lebedev.ri added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4160
+  // And spill it into the stack slot.
+  Ch = DAG.getStore(Ch, dl, Init, StackPtr, MachinePointerInfo());
+
----------------
efriedma wrote:
> lebedev.ri wrote:
> > craig.topper wrote:
> > > efriedma wrote:
> > > > lebedev.ri wrote:
> > > > > efriedma wrote:
> > > > > > MachinePointerInfo::getUnknownStack instead of MachinePointerInfo()?  Or actually, DAGTypeLegalizer::SplitVecRes_INSERT_SUBVECTOR does the following:
> > > > > > 
> > > > > > ```
> > > > > >   auto FrameIndex = cast<FrameIndexSDNode>(StackPtr.getNode())->getIndex();
> > > > > >   auto PtrInfo = MachinePointerInfo::getFixedStack(MF, FrameIndex);
> > > > > > ```
> > > > > As per @craig.topper, and looking at `-debug`,
> > > > > we get right `MachinePointerInfo` for stack-based
> > > > > memory operations automatically. Not doing that manually
> > > > > frees us from having to track alignment, too.
> > > > I guess optimizations kick in, sure, so you don't need to compute the exact pointer info.
> > > > 
> > > > But please at least use getUnknownStack(); a default-constructed MachinePointerInfo implicitly points to address-space zero.
> > > Isn't there a check for null pointer info in getLoad/getStore to infer the pointer info? Will that still trigger with getUnknownStack()?
> > Yup: https://github.com/llvm/llvm-project/blob/62fc5f16405a7d39e62044bc461752f3f31bdca0/llvm/lib/CodeGen/SelectionDAG/SelectionDAG.cpp#L8007-L8010
> Having an interface that says "the address-space of the store is based on the MachinePointerInfo()... unless the address refers to a frame index, in which case we ignore the MachinePointerInfo() you passed" is not at all intuitive.  The fact that it works that way looks like a historical accident.  Please don't abuse this behavior; if you want a more convenient interface for a store to a frame index, please add a new interface.
FWIW, originally i did exactly what `DAGTypeLegalizer::SplitVecRes_INSERT_SUBVECTOR()` did, 
but @craig.topper noted that it happens automatically. 


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140638