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

[Oren Ben Simhon via llvm-commits]

oren_ben_simhon marked 8 inline comments as done.
oren_ben_simhon added inline comments.


================
Comment at: lib/Target/X86/X86CallingConv.cpp:43
+  if (AvailableRegs.size() < REQUIRED_GPRS_UPON_SPLIT)
+    return false; // Not enough free registers - continue the search.
+
----------------
delena wrote:
> How the case is handled when there is no available registers? I did not see assign-to stack for this type.
Since v64i1 is promoted to i64, the following rule will assign to stack (in case we don't have available registers):

    // In 64 bit, assign 64/32 bit values to 8 byte stack
    CCIfSubtarget<"is64Bit()", CCIfType<[i32, i64, f32, f64], 
      CCAssignToStack<8, 8>>>,


================
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">>,
+
----------------
delena wrote:
> do you implement CC_X86_RegCall_Error for the types that will come in the next commit?
Yes


================
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,
----------------
delena wrote:
> you can combine this "if" with the previous one, right?
I can do so but it will require duplication of the following lines:
  // Convert the i32 type into v32i1 type
  Lo = DAG.getBitcast(MVT::v32i1, ArgValue);
...
  // Convert the i32 type into v32i1 type
  Hi = DAG.getBitcast(MVT::v32i1, ArgValue);

So i prefered to seprate it into two different if statements.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2438
+
+  // Currently not referenced - will be used in other mask lowering
+  (void)Dl;
----------------
delena wrote:
> compilation will fail for unreferenced variable in the buildbot.
It is referenced in the next line. It will be referenced in the future as well.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2445
+  if (ValVT == MVT::v64i1 && ValLoc == MVT::i32) {
+    llvm_unreachable("Expecting only i64 locations");
+    return;
----------------
delena wrote:
> unreachable or assert - ?
This decision was made due to future design.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:2495
+      Val =
+          Getv64i1Argument(VA, RVLocs[++I], Chain, DAG, dl, Subtarget, &InFlag);
+    } else {
----------------
delena wrote:
> wrong line alignment, function name lower case
The alignment was created by clang-format.
I will move to lower case.


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


================
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
----------------
delena wrote:
> I don't understand this change
LLVM Coding Standards prefer all variables to be capitalized including iterator.
They share the following example:

    for (unsigned I = 0, E = List.size(); I != E; ++I)
        if (List[I]->isFoo())
          return true;


================
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()) {
----------------
delena wrote:
> Why do you drop SIGN_EXTEND?
> 
It resides inside the function


Repository:
  rL LLVM

https://reviews.llvm.org/D26181