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

[Elena Demikhovsky via llvm-commits]

delena added inline comments.

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:521
@@ +520,3 @@
+  SDLoc dl(N);
+  SDValue Ops[] = {N->getChain(), ExtSrc0, N->getMask(), N->getBasePtr(),
+                   N->getIndex()};
----------------
mbodart wrote:
> 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.
No reason. Just the old code. I removed the mask promotion and all tests still pass.

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:1239
@@ +1238,3 @@
+    NewOps[OpNo] = PromoteTargetBoolean(N->getOperand(OpNo), DataVT);
+  } else
+    NewOps[OpNo] = GetPromotedInteger(N->getOperand(OpNo));
----------------
mbodart wrote:
> There appears to be an extraneous blank line here.
I did not see any blank line. I'll run dos2unix on all files again.

================
Comment at: ../lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:1115
@@ +1114,3 @@
+  if (getTypeAction(Bool.getValueType()) == TargetLowering::TypePromoteInteger)
+    Bool = GetPromotedInteger(Bool);
+
----------------
mbodart wrote:
> 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?
Fixed! Thank you.

================
Comment at: ../lib/Target/X86/X86ISelLowering.cpp:19738
@@ +19737,3 @@
+
+  if (MemVT.getScalarSizeInBits() < VT.getScalarSizeInBits()) {
+    // Promoted data type
----------------
mbodart wrote:
> 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?
This is the case of promotion of v2i32 to v2i64. Only MemVT keeps the original VT. I'm actually "redo" the TypeLegalizer's work.
The type legalizer promoted v2i32 to v2i64 and I retrieve v2i32 with shuffle {0, 2, -1, -1} and then widen the result  to v4i32.
I'll add comments.


Repository:
  rL LLVM

http://reviews.llvm.org/D13633