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

[Mitch Bodart via llvm-commits]

mbodart added a comment.

Hi Elena,

I added just a few more comments in line.

regards,

- mitch


================
Comment at: ../include/llvm/CodeGen/SelectionDAGNodes.h:2116
@@ -2115,3 +2115,3 @@
     assert(getValue().getValueType() == getValueType(0) &&
            "Incompatible type of the PathThru value in MaskedGatherSDNode");
     assert(getMask().getValueType().getVectorNumElements() ==
----------------
Not your change, but minor typo:  PathThru => PassThru

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:521
@@ +520,3 @@
+  SDLoc dl(N);
+  SDValue Ops[] = {N->getChain(), ExtSrc0, N->getMask(), N->getBasePtr(),
+                   N->getIndex()};
----------------
Is there any particular reason that the mask operand legalization is handled in a different place for MLOAD and MGATHER?  For MLOAD it is processed during PromoteIntRes_MLOAD, while for MGATHER it is processed when legalizing its operands.  The MGATHER method seems cleaner, but I'm curious as to why MLOAD does it differently.  Also note that for MLOAD, the call to PromoteTargetBoolean is conditioned on whether the type mismatches, while in PromoteIntOp_MGATHER, the call is unconditional.  I don't know if these inconsistencies are innocuous from a functionality standpoint, but they certainly make it difficult to understand the requirements.

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:1239
@@ +1238,3 @@
+    NewOps[OpNo] = PromoteTargetBoolean(N->getOperand(OpNo), DataVT);
+  } else
+    NewOps[OpNo] = GetPromotedInteger(N->getOperand(OpNo));
----------------
There appears to be an extraneous blank line here.

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:1115
@@ +1114,3 @@
+  if (getTypeAction(Bool.getValueType()) == TargetLowering::TypePromoteInteger)
+    Bool = GetPromotedInteger(Bool);
+
----------------
It seems important here that the value returned by GetPromotedInteger matches the promotion operation (e.g.. sign extension) performed by PromoteTargetBoolean.
How is that guaranteed?

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeTypes.h:177
@@ +176,3 @@
+
+  /// Modify Bit Vector to match SetCC result type if ValVT.
+  /// The bit vector is widened with zeroes when WithZeroes is true.
----------------
Should that be "of ValVT", not "if"?

================
Comment at: ../lib/Target/X86/X86ISelLowering.cpp:19738
@@ +19737,3 @@
+
+  if (MemVT.getScalarSizeInBits() < VT.getScalarSizeInBits()) {
+    // Promoted data type
----------------
It seems odd to me to allow a masked store/scatter where the value element size differs from the memory element size.  Is this an unavoidable result of type legalization?  What are the semantics of such an operation?  If the value is i64 and the memory is i32, apparently we just store the low 32 bits of each i64 element.  Is that the expected behavior?  Do we ever have to worry about a size difference in the opposite direction, e.g. storing an i32 to an i64?

================
Comment at: ../test/CodeGen/X86/masked_gather_scatter.ll:1244
@@ -318,9 +1243,3 @@
 declare <3 x i32> @llvm.masked.gather.v3i32(<3 x i32*>, i32, <3 x i1>, <3 x i32>)
-; KNL-LABEL: test16
-; KNL: testb
-; KNL: je
-; KNL: testb
-; KNL: je
-; KNL: testb
-; KNL: je
-define <3 x i32> @test16(<3 x i32*> %base, <3 x i32> %ind, <3 x i1> %mask, <3 x i32> %src0) {
+define <3 x i32> @test30(<3 x i32*> %base, <3 x i32> %ind, <3 x i1> %mask, <3 x
+; KNL_64-LABEL: test30:
----------------
Please keep the last <3 x i32> all on the same line.


Repository:
  rL LLVM

http://reviews.llvm.org/D13633