[PATCH] D140638: [Codegen][LegalizeIntegerTypes] New legalization strategy for scalar shifts w/ shift amount by a multiple of CHAR_BIT

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:187
   SDValue ExpandVectorBuildThroughStack(SDNode* Node);
+  SDValue ExpandScalarSrlThroughStack(SDNode *Node);
 
----------------
Unused?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4120
+  EVT ShAmtVT = ShAmt.getValueType();
+  uint64_t ShAmtVTBitWidth = ShAmtVT.getScalarSizeInBits();
+  assert(ShAmtVTBitWidth % 8 == 0 && "Shift amount type is not byte multiple?");
----------------
Unused variable in release builds


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4120
+  EVT ShAmtVT = ShAmt.getValueType();
+  uint64_t ShAmtVTBitWidth = ShAmtVT.getScalarSizeInBits();
+  assert(ShAmtVTBitWidth % 8 == 0 && "Shift amount type is not byte multiple?");
----------------
craig.topper wrote:
> Unused variable in release builds
`unsigned`


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4128
+
+  uint64_t VTBitWidth = VT.getScalarSizeInBits();
+  assert(VTBitWidth % 8 == 0 && "Shifting a not byte multiple value?");
----------------
Can these be unsigned? I'm not sure why getScalarSizeInBits returns uint64_t. The SizeInBits methods in Datalayout and Type return unsigned.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4165
+        DAG.getSetCC(dl, MVT::i1, Shiftee, AllZeros, ISD::SETLT);
+    Padding = DAG.getNode(ISD::SIGN_EXTEND, dl, VT, ShifteeIsNegative);
+  }
----------------
Can this be SRA of Shiftee by VTBits-1?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:4182
+  // NOTE: we can clamp to either VTByteWidth or VTByteWidth-1.
+  ByteOffset = DAG.getNode(ISD::UMIN, dl, ShAmtVT, ByteOffset,
+                           DAG.getConstant(VTByteWidth, dl, ShAmtVT));
----------------
Can this be clipped by masking with AND?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D140638