[PATCH] D115653: [DAG]Introduce llvm::processShuffleMasks and use it for shuffles in DAG Type Legalizer.

[Alexey Bataev via Phabricator via llvm-commits]

ABataev added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2042
   SDValue Inputs[4];
-  SDLoc dl(N);
+  SDLoc Dl(N);
   GetSplitVector(N->getOperand(0), Inputs[0], Inputs[1]);
----------------
craig.topper wrote:
> If you change the case here. Use `DL`. As far as I know SelectionDAG uses `DL` or `dl`.
Ok, will rename it to `DL`


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2055
     // Build a shuffle mask for the output, discovering on the fly which
     // input vectors to use as shuffle operands (recorded in InputUsed).
     unsigned FirstMaskIdx = High * NewElts;
----------------
craig.topper wrote:
> InputUsed no longer exists
Will be fixed.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2063
+        /*NumOfUsedRegs=*/1,
+        [&Output, this, NewVT]() { Output = DAG.getUNDEF(NewVT); },
+        [&Output, this, NewVT, &Dl, &Inputs](ArrayRef<int> Mask, unsigned Idx) {
----------------
craig.topper wrote:
> Is `this` only used for DAG?
Yes


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2070
+         &InitManyRegs](ArrayRef<int> Mask, unsigned Idx1, unsigned Idx2) {
+          if (InitManyRegs) {
+            Output = Inputs[Idx1];
----------------
craig.topper wrote:
> Can this use `SDValue::operator bool()` to see if it has already been assigned instead of using a separate flag.
I'll check if it can be improved


================
Comment at: llvm/lib/Target/ARM/ARMISelLowering.cpp:394
     setOperationAction(ISD::BUILD_VECTOR, VT, Custom);
+    setOperationAction(ISD::VECTOR_SHUFFLE, VT, Custom);
     setOperationAction(ISD::VSELECT, VT, Legal);
----------------
craig.topper wrote:
> Does ARM already have code that just does the right thing for these types?
Actually, need advice here. Without this change, there is an infinite loop for ARM tests for `v2f64` type. VECTOR_SHUFFLE is assigned Expand action, so it is expanded, then it is transformed back to vector (`setOperationAction(ISD::SCALAR_TO_VECTOR, MVT::v2f64, Legal);`), then again expanded, etc. I assume that it is legal for `v2f64` type if `ISD::SCALAR_TO_VECTOR` is legal. But better to have some input here from ARM folks.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D115653