[FastIsel][X86] Fix invalid register replacement for bool args

[loladiro]

Summary

Consider the following IR:

%3 = load i8* undef
%4 = trunc i8 %3 to i1
%5 = call %jl_value_t.0* @foo(..., i1 %4, ...)
ret %jl_value_t.0* %5

Bools (that are the result of direct truncs) are lowered as whatever
the argument to the trunc was and a "and 1", causing the part of the
MBB responsible for this argument to look something like this:

%vreg8<def,tied1> = AND8ri %vreg7<kill,tied0>, 1, %EFLAGS<imp-def>; GR8:%vreg8,%vreg7

Later, when the load is lowered, it will insert

%vreg15<def> = MOV8rm %vreg14, 1, %noreg, 0, %noreg; mem:LD1[undef] GR8:%vreg15 GR64:%vreg14

but remember to (at the end of isel) replace vreg7 by vreg15. Now for
the bug. In fast isel lowering, we mistakenly mark vreg8 as the result
of the load instead of the trunc. This adds a fixup to have
vreg8 replaced by whatever the result of the load is as well, so
we end up with

%vreg15<def,tied1> = AND8ri %vreg15<kill,tied0>, 1, %EFLAGS<imp-def>; GR8:%vreg15

which is an SSA violation and causes problems later down the road.

This fixes PR21557.

[ributzka]

Hi Keno,

thanks for catching this. I am a little surprised this hasn't shown up earlier. I only have two minor tweaks inline.

The test case looks too verbose for what it is testing. Could you please trim it down to the bare essentials?

Thanks

Cheers,
Juergen

lib/Target/X86/X86FastISel.cpp

2698 ↗

(On Diff #16138)

const Value *PrevVal = TI->getOperand(0);

2709 ↗

(On Diff #16138)

fastEmit_ri(ArgVT, ArgVT, ISD::AND, ResultReg, hasTrivialKill(PrevVal), 1);

[loladiro]

Address review comments

[ributzka]

Great. Thanks for fixing this.

Could you please also add a FileCheck to the new test case before you commit?

LGTM.

[loladiro]

Hmm, I'm not sure what there really is to filecheck. Just a CHECK: callq?

[ributzka]

Checking for the label, the 'load' and the 'and' would be nice too ;-)

[loladiro]

The load and and are of an undef so they won't be there.

[ributzka]

You could make it a load from an address. The address could be simply an argument to 'foo'.

[loladiro]

Unfortunately, this patch isn't quite correct. Since the OutVals[i] isn't updated it'll bail out in the next loop, since it thinks the argument is an i1, which isn't legal. However, even after fixing that the trunc is now in the value map, causing it to get put into FuncInfo.ValueMap and hence subsequently isel'd (which we don't want, because we already emit the code for it). Any idea how to fix that?

[loladiro]

Worked around the issue by never inserting Val into the ValueMap. This does require keeping track of the appropriate call register separately, but I think that's fine. Thoughts?

[ributzka]

Hi Keno,

this looks good. Thanks for fixing this.

Cheers,
Juergen

lib/Target/X86/X86FastISel.cpp

2698 ↗

(On Diff #16189)

unsigned ResultReg;

2714–2717 ↗

(On Diff #16189)

Move this after the end of the "else" block.

2721 ↗

(On Diff #16189)

ResultReg = getRegForValue(Val);

2757–2758 ↗

(On Diff #16189)

This check could be now removed.

test/CodeGen/X86/fast-isel-call-bool.ll

7 ↗

(On Diff #16189)

delete

[loladiro]

I totally forgot about this. Will address comments, rebase and commit.

llvm/trunk/lib/Target/X86/X86FastISel.cpp

bool X86FastISel::fastLowerCall(CallLoweringInfo &CLI) {
   if (CLI.CS && CLI.CS->hasInAllocaArgument())
     return false;
 
   // Fast-isel doesn't know about callee-pop yet.
   if (X86::isCalleePop(CC, Subtarget->is64Bit(), IsVarArg,
                        TM.Options.GuaranteedTailCallOpt))
     return false;
 
+  SmallVector<MVT, 16> OutVTs;
+  SmallVector<unsigned, 16> ArgRegs;
+
   // If this is a constant i1/i8/i16 argument, promote to i32 to avoid an extra
   // instruction. This is safe because it is common to all FastISel supported
   // calling conventions on x86.
   for (int i = 0, e = OutVals.size(); i != e; ++i) {
     Value *&Val = OutVals[i];
     ISD::ArgFlagsTy Flags = OutFlags[i];
     if (auto *CI = dyn_cast<ConstantInt>(Val)) {
       if (CI->getBitWidth() < 32) {
         if (Flags.isSExt())
           Val = ConstantExpr::getSExt(CI, Type::getInt32Ty(CI->getContext()));
         else
           Val = ConstantExpr::getZExt(CI, Type::getInt32Ty(CI->getContext()));
       }
     }
 
     // Passing bools around ends up doing a trunc to i1 and passing it.
     // Codegen this as an argument + "and 1".
-    if (auto *TI = dyn_cast<TruncInst>(Val)) {
-      if (TI->getType()->isIntegerTy(1) && CLI.CS &&
+    MVT VT;
+    auto *TI = dyn_cast<TruncInst>(Val);
+    unsigned ResultReg;
+    if (TI && TI->getType()->isIntegerTy(1) && CLI.CS &&
               (TI->getParent() == CLI.CS->getInstruction()->getParent()) &&
               TI->hasOneUse()) {
-        Val = cast<TruncInst>(Val)->getOperand(0);
-        unsigned ResultReg = getRegForValue(Val);
+      Value *PrevVal = TI->getOperand(0);
+      ResultReg = getRegForValue(PrevVal);
 
-        if (!ResultReg)
-          return false;
 
-        MVT ArgVT;
-        if (!isTypeLegal(Val->getType(), ArgVT))
-          return false;
+      if (!isTypeLegal(PrevVal->getType(), VT))
 
-        ResultReg =
-          fastEmit_ri(ArgVT, ArgVT, ISD::AND, ResultReg, Val->hasOneUse(), 1);
+        fastEmit_ri(VT, VT, ISD::AND, ResultReg, hasTrivialKill(PrevVal), 1);
 
-        if (!ResultReg)
-          return false;
-        updateValueMap(Val, ResultReg);
-      }
+    } else {
+      if (!isTypeLegal(Val->getType(), VT))
+        return false;
+      ResultReg = getRegForValue(Val);
     }
+
+    ArgRegs.push_back(ResultReg);
+    OutVTs.push_back(VT);
   }
 
   // Analyze operands of the call, assigning locations to each operand.
   SmallVector<CCValAssign, 16> ArgLocs;
   CCState CCInfo(CC, IsVarArg, *FuncInfo.MF, ArgLocs, CLI.RetTy->getContext());
 
   // Allocate shadow area for Win64
   if (IsWin64)
     CCInfo.AllocateStack(32, 8);
 
-  SmallVector<MVT, 16> OutVTs;
-  for (auto *Val : OutVals) {
-    MVT VT;
-    if (!isTypeLegal(Val->getType(), VT))
-      return false;
-    OutVTs.push_back(VT);
-  }
   CCInfo.AnalyzeCallOperands(OutVTs, OutFlags, CC_X86);
 
   // Get a count of how many bytes are to be pushed on the stack.
   unsigned NumBytes = CCInfo.getNextStackOffset();
 
   // Issue CALLSEQ_START
   unsigned AdjStackDown = TII.getCallFrameSetupOpcode();
   BuildMI(*FuncInfo.MBB, FuncInfo.InsertPt, DbgLoc, TII.get(AdjStackDown))
     .addImm(NumBytes);
 
   // Walk the register/memloc assignments, inserting copies/loads.
   const X86RegisterInfo *RegInfo = static_cast<const X86RegisterInfo *>(
       TM.getSubtargetImpl()->getRegisterInfo());
   for (unsigned i = 0, e = ArgLocs.size(); i != e; ++i) {
     CCValAssign const &VA = ArgLocs[i];
     const Value *ArgVal = OutVals[VA.getValNo()];
     MVT ArgVT = OutVTs[VA.getValNo()];
 
     if (ArgVT == MVT::x86mmx)
       return false;
 
-    unsigned ArgReg = getRegForValue(ArgVal);
-    if (!ArgReg)
-      return false;
+    unsigned ArgReg = ArgRegs[VA.getValNo()];
 
     // Promote the value if needed.
     switch (VA.getLocInfo()) {
     case CCValAssign::Full: break;
     case CCValAssign::SExt: {
       assert(VA.getLocVT().isInteger() && !VA.getLocVT().isVector() &&
              "Unexpected extend");
       bool Emitted = X86FastEmitExtend(ISD::SIGN_EXTEND, VA.getLocVT(), ArgReg,

llvm/trunk/test/CodeGen/X86/fast-isel-call-bool.ll

+; RUN: llc < %s -fast-isel -mcpu=core2 -O1 | FileCheck %s
+; See PR21557
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-darwin14.0.0"
+
+declare i64 @bar(i1)
+
+define i64 @foo(i8* %arg) {
+; CHECK-LABEL: foo:
+top:
+  %0 = load i8* %arg
+; CHECK: movb
+  %1 = trunc i8 %0 to i1
+; CHECK: andb $1,
+  %2 = call i64 @bar(i1 %1)
+; CHECK: callq
+  ret i64 %2
+}