[PATCH] D26181: RegCall - Handling v64i1 in 32/64 bit target

[Elena Demikhovsky via llvm-commits]

delena added inline comments.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:26
+                                   CCState &State) {
+#define REQUIRED_GPRS_UPON_SPLIT 2
+
----------------
#defines like this are not allowed in LLVM, I think.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:43
+  if (AvailableRegs.size() < REQUIRED_GPRS_UPON_SPLIT)
+    return false; // Not enough free registers - continue the search.
+
----------------
How the case is handled when there is no available registers? I did not see assign-to stack for this type.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:49
+    // Marking the register as located
+    unsigned reg = State.AllocateReg(AvailableRegs[I]);
+
----------------
All variables names should start with capital letter


================
Comment at: lib/Target/X86/X86CallingConv.td:91
+    // TODO: Handle the case of mask types (v*i1)
+    CCIfType<[v8i1, v16i1, v32i1], CCCustom<"CC_X86_RegCall_Error">>,
+
----------------
do you implement CC_X86_RegCall_Error for the types that will come in the next commit?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2372
+/// Reads two 32 bit registers and creates a 64 bit mask value
+static SDValue Getv64i1Argument(CCValAssign &VA, CCValAssign &NextVA,
+                                SDValue &Root, SelectionDAG &DAG,
----------------
Please add 2 words about the Root, VA and NextVA arguments in doxygen format. And InFlag role is not clear.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2376
+                                SDValue *InFlag = nullptr) {
+  assert((Subtarget.hasBWI() || Subtarget.hasBMI()) &&
+         "Expected AVX512BW or AVX512BMI target!");
----------------
BMI implies BWI


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2410
+  // Read a 32 bit value from the register
+  if (nullptr == InFlag) {
+    // When no physical register is present,
----------------
you can combine this "if" with the previous one, right?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2431
+static void LowerRegToMasks(SDValue &ValReturned, const EVT &ValVT,
+                            const EVT &ValLoc, CallingConv::ID CallConv,
+                            const SDLoc &Dl, SelectionDAG &DAG) {
----------------
do not pass calling convention here


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2438
+
+  // Currently not referenced - will be used in other mask lowering
+  (void)Dl;
----------------
compilation will fail for unreferenced variable in the buildbot.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2445
+  if (ValVT == MVT::v64i1 && ValLoc == MVT::i32) {
+    llvm_unreachable("Expecting only i64 locations");
+    return;
----------------
unreachable or assert - ?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2449
+
+  ValReturned = DAG.getBitcast(ValVT, ValReturned);
+}
----------------
The function may return SDValue. But you do many asserts and one bitcast. Do you really need a function?


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2495
+      Val =
+          Getv64i1Argument(VA, RVLocs[++I], Chain, DAG, dl, Subtarget, &InFlag);
+    } else {
----------------
wrong line alignment, function name lower case


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2515
+      } else
+        Val = DAG.getNode(ISD::TRUNCATE, dl, VA.getValVT(), Val);
+    }
----------------
the original condition for this operation was VA.getValVT().getScalarType() == MVT::i1


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2644
+      VA.isExtInLoc() && VA.getValVT().getScalarType() == MVT::i1 &&
+      VA.getValVT().getSizeInBits() != VA.getLocVT().getSizeInBits();
 
----------------
Why do we need this additional condition? Please add comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2769
+  assert(
+      !(isVarArg && canGuaranteeTCO(CallConv)) &&
+      "Var args not supported with calling conv' regcall, fastcc, ghc or hipe");
----------------
wrong line alignment


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2882
 
-  for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
+  for (unsigned I = 0, E = Ins.size(); I != E; ++I) {
     // Swift calling convention does not require we copy the sret argument
----------------
I don't understand this change


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:3317
           Arg.getValueType().getVectorElementType() == MVT::i1)
-        Arg = DAG.getNode(ISD::SIGN_EXTEND, dl, RegVT, Arg);
+        LowerMasksToReg(Arg, RegVT, CallConv, dl, DAG);
       else if (RegVT.is128BitVector()) {
----------------
Why do you drop SIGN_EXTEND?



Repository:
  rL LLVM

https://reviews.llvm.org/D26181