[PATCH] D13633: Type legalizer for masked gather/scatter intrinsics

[Mitch Bodart via llvm-commits]

mbodart added inline comments.

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:503
@@ +502,3 @@
+  assert(NVT == ExtSrc0.getValueType() &&
+      "Gather result type and the passThru agrument type should be ther same");
+
----------------
minor typos: "ther" => "the"

And a little below, and in several other places in this file/change set:  "Legalized the chain result ..." => "Legalize the chain result ..."

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:1235
@@ +1234,3 @@
+
+SDValue DAGTypeLegalizer::PromoteIntOp_MSCATTER(MaskedScatterSDNode *N,
+                                                unsigned OpNo) {
----------------
This is the first time I've looked at vector type legalization, so there's something I don't quite understand.  A masked store (PromoteIntOp_MSTORE) is in some sense just a degenerative case of a masked scatter.  So intuitively it would seem PromoteIntOp_MSTORE would have a more simple implementation than PromoteIntOp_MSCATTER.  But here it seems the reverse is true.  Why does PromoteIntOp_MSTORE try to legalize both the mask and data simultaneously, while for scatter we just do the one operand?  Could PromoteIntOp_MSTORE be simplified?  If not, what is the essential difference?

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeTypes.h:744
@@ -738,3 +743,3 @@
 
   /// Modifies a vector input (widen or narrows) to a vector of NVT.  The
   /// input vector must have the same element type as NVT.
----------------
Can you please change the parameter name from WidenVT to NVT, so that it matches this comment and the code in LegalizeVectorTypes.cpp?

Also a typo:  defalut => default

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeTypes.h:746-751
@@ -740,4 +745,8 @@
   /// input vector must have the same element type as NVT.
-  SDValue ModifyToType(SDValue InOp, EVT WidenVT);
-
+  /// When FillWithZeroes is "on" the vector will be widened with
+  /// zeroes. UseExistingVal means that the existing elements from InOp
+  /// should be used for the widening.
+  /// By defalut, the vector will be widened with undefined values.
+  SDValue ModifyToType(SDValue InOp, EVT WidenVT, bool FillWithZeroes = false,
+                       bool UseExistingVal = false);
 
----------------
The implementation of UseExistingVal gives it an ambiguous meaning.  If a widening operation is an even multiple of the original size, then the full original operand is replicated to fill out the new value.  Otherwise, only its first element is replicated.  Was that the intent?  If so, it seems like a difficult interface to use robustly.

An enum with possible values of Zext, Splat or Undef would make the widening interface
more clear, assuming the Splat ambiguity is resolved to consistently replicate all original values.

But as a further question, when would a larger vector length not be a multiple of a smaller one?
Aren't they all powers of 2?


================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:2704-2714
@@ -2708,11 +2703,13 @@
 
-    unsigned NumConcat = WidenNumElts / MaskNumElts;
-    SmallVector<SDValue, 16> Ops(NumConcat);
-    SDValue ZeroVal = DAG.getConstant(0, dl, MaskVT);
-    Ops[0] = Mask;
-    for (unsigned i = 1; i != NumConcat; ++i)
-      Ops[i] = ZeroVal;
+  unsigned WidenNumElts = BoolVT.getVectorNumElements();
+  unsigned MaskNumElts = MaskVT.getVectorNumElements();
 
-    Mask = DAG.getNode(ISD::CONCAT_VECTORS, dl, BoolVT, Ops);
-  }
+  unsigned NumConcat = WidenNumElts / MaskNumElts;
+  SmallVector<SDValue, 16> Ops(NumConcat);
+  SDValue ZeroVal = DAG.getConstant(0, dl, MaskVT);
+  Ops[0] = Mask;
+  for (unsigned i = 1; i != NumConcat; ++i)
+    Ops[i] = ZeroVal;
+
+  Mask = DAG.getNode(ISD::CONCAT_VECTORS, dl, BoolVT, Ops);
 
----------------
Now that ModifyToType supports zero fill, why aren't we using it here (and in WidenVecOp_MSTORE)?

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeVectorTypes.cpp:3663-3664
@@ -3599,1 +3662,4 @@
          "input and widen element type must match");
+  if (InOp.isUndef())
+    return DAG.getUNDEF(NVT);
+
----------------
I would think we want this moved below the check for "if (InVT == NVT)".

================
Comment at: ../lib/Target/X86/X86ISelLowering.cpp:1595-1596
@@ -1594,2 +1594,4 @@
         setOperationAction(ISD::MSTORE,              VT, Legal);
+        setOperationAction(ISD::MGATHER,             VT, Legal);
+        setOperationAction(ISD::MSCATTER,            VT, Custom);
       }
----------------
Why would 512-bit gathers and scatters have different operation actions here?

================
Comment at: ../lib/Target/X86/X86ISelLowering.cpp:19510
@@ -19507,1 +19509,3 @@
 
+/// Modifies a vector input (widen or narrows) to a vector of NVT.  The
+/// input vector must have the same element type as NVT.
----------------
It's unfortunate that we have to duplicate much of the functionality of the generic type modifier in CodeGen's ModifyToType.

Is there a way to unify them?

If not, please add a source comment describing how this implementation differs from the one in CodeGen.

================
Comment at: ../test/CodeGen/X86/masked_gather_scatter.ll:2
@@ -2,1 +1,3 @@
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu  -mattr=+avx512f < %s | FileCheck %s --check-prefix=ALL --check-prefix=KNL
+; RUN: llc -mtriple=x86_64-unknown-linux-gnu  -mattr=+avx512vl -mattr=+avx512dq < %s | FileCheck %s --check-prefix=ALL --check-prefix=SKX
 
----------------
Have you tried your changes with gathers/scatters targetting 32-bit X86?

Poking around in the X86 test area, it seems all triples use x86_64 for gather/scatter tests.


Repository:
  rL LLVM

http://reviews.llvm.org/D13633