[PATCH] D146357: [X86] Alternative algorithm for vector-vector shifts

[Simon Pilgrim via Phabricator via llvm-commits]

RKSimon edited reviewers, added: RKSimon, pengfei, LuoYuanke; removed: LLVM.
RKSimon added a comment.

It might be better to split the rotate/funnel and general shift lowering into 2 separate patches.



================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31023
+  // Alternative version of vector-vector shifts for certain types.
+  // Should be more effective with slow cross-lane moves.
+  if ((Subtarget.hasInt256() && !DAG.isSplatValue(Amt) && !Subtarget.hasBWI() &&
----------------
Describe the codegen?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31028
+       ((Subtarget.hasVLX() && (VT == MVT::v16i8 || VT == MVT::v32i8)) ||
+        VT == MVT::v64i8))) {
+    MVT ExtST = VT.getScalarType() == MVT::i8 ? MVT::i16 : MVT::i32;
----------------
This logic should be something like if !supportedVectorVarShift(VT) && supportedVectorVarShift(WideVT) ?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31041
+    R = DAG.getBitcast(ExtVT, R);
+    RH = DAG.getBitcast(ExtVT, R);
+    MaskH = DAG.getConstant(MaskValue, dl, Ext32);
----------------
Lo/Hi might be better names?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31044
+    AH = DAG.getBitcast(ExtVT, Amt);
+    AH = DAG.getNode(ISD::SRL, dl, ExtVT, AH, DAG.getConstant(ShC, dl, ExtVT));
+
----------------
getTargetVShiftByConstNode ?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31046
+
+    if (DAG.isSplatValue(Amt)) {
+      Amt = AH;
----------------
Don't you check for !DAG.isSplatValue(Amt) in the opening if() ?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31054
+
+    if (Op->getOpcode() == ISD::SHL) {
+      RH = DAG.getBitcast(Ext32, R);
----------------
LowerShift has a unsigned Opc we can use


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31077
+                      DAG.getBitcast(Ext32, R), MaskH,
+                      DAG.getTargetConstant(0xe4, dl, MVT::i8));
+    } else if (VT.getScalarType() == MVT::i8) {
----------------
Do we need the ternlog path? Don't existing combines deal with it later (canonicalizeBitSelect etc.)?


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31454
+    // See below.
+    if ((Subtarget.hasInt256() && !DAG.isSplatValue(Amt) && !IsCst &&
+         !Subtarget.hasBWI() && VT.getScalarType() == MVT::i16) ||
----------------
It'd be better if you can adjust the logic below instead of having a second copy.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31667
+
+  // See below.
+  if ((Subtarget.hasInt256() && !DAG.isSplatValue(Amt) && !IsConstAmt &&
----------------
See below what? Add a new self-contained comment instead of relying on a code layout.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31680
+    return getPack(DAG, Subtarget, DL, VT, Lo, Hi, IsROTL);
+    }
+
----------------
clang-format (indentation)


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:31693
     if (supportedVectorVarShift(WideVT, Subtarget, ShiftOpc) &&
         supportedVectorShiftWithImm(WideVT, Subtarget, ShiftOpc)) {
       // If we're rotating by constant, just use default promotion.
----------------
Same for funnel shifts - by tweaking this logic we might be able to avoid the duplicate code.


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

https://reviews.llvm.org/D146357