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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4134
+  unsigned VTByteWidth = VTBitWidth / 8;
+  EVT ByteVecVT = EVT::getVectorVT(
+      *DAG.getContext(), EVT::getIntegerVT(*DAG.getContext(), 8), VTByteWidth);
----------------
Is this ByteVecVT only used to make clampDynamicVectorIndex work? It won't cause vector instructions to be generated from scalar code will it?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4164
+  // And spill it into the stack slot.
+  Ch = DAG.getStore(Ch, dl, Init, StackPtr, StackPtrInfo);
+
----------------
I think the alignment is incorrect on this store.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4176
+  // while shift overflow would have *just* been poison.
+  ByteOffset = TargetLowering::clampDynamicVectorIndex(
+      DAG, ByteOffset, ByteVecVT, dl,
----------------
Is this always going to create an AND? Trying to decide if bringing "Vector" into this made this more confusing.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4190
+  SDValue AdjStackPtr;
+  if (WillIndexUpwards)
+    AdjStackPtr = StackPtr;
----------------
Nit: Use curly braces for consistency with `else`


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4212
+                                   DAG.getConstant(7, dl, ShAmtVT));
+    Res = DAG.getNode(N->getOpcode(), dl, VT, Res, ShAmtRem);
+  }
----------------
Does this shift end up in `ExpandShiftWithKnownAmountBit` because of the AND?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140638