[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

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

2709

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

2703

unsigned ResultReg;

2714–2715

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

2721

ResultReg = getRegForValue(Val);

2755–2756

This check could be now removed.

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

8

delete

[loladiro]

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

lib/Target/X86/X86FastISel.cpp

for (int i = 0, e = OutVals.size(); i != e; ++i) {
     }
 
     // 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 &&
           (TI->getParent() == CLI.CS->getInstruction()->getParent()) &&
           TI->hasOneUse()) {
-        Val = cast<TruncInst>(Val)->getOperand(0);
-        unsigned ResultReg = getRegForValue(Val);
+        Value *PrevVal = cast<TruncInst>(Val)->getOperand(0);
+        unsigned ResultReg = getRegForValue(PrevVal);
 
         if (!ResultReg)
           return false;
 
         MVT ArgVT;
-        if (!isTypeLegal(Val->getType(), ArgVT))
+        if (!isTypeLegal(PrevVal->getType(), ArgVT))
           return false;
 
         ResultReg =
-          fastEmit_ri(ArgVT, ArgVT, ISD::AND, ResultReg, Val->hasOneUse(), 1);
+          fastEmit_ri(ArgVT, ArgVT, ISD::AND, ResultReg, PrevVal->hasOneUse(), 1);
 
         if (!ResultReg)
           return false;
         updateValueMap(Val, ResultReg);
       }
     }
   }
 
   // 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))
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;
 
     // 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,

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

+; RUN: llc < %s -fast-isel -mcpu=core2 -O1
+; See PR21557
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-darwin14.0.0"
+
+%jl_value_t.0 = type { %jl_value_t.0* }
+
+; Function Attrs: sspreq
+declare %jl_value_t.0* @julia_call19(%jl_value_t.0*, %jl_value_t.0*, %jl_value_t.0*, %jl_value_t.0*, i64, %jl_value_t.0*, i1, i64, double) #0
+
+define %jl_value_t.0* @jlcall_call19(%jl_value_t.0*, %jl_value_t.0**, i32) {
+top:
+  %3 = load i8* undef
+  %4 = trunc i8 %3 to i1
+  %5 = call %jl_value_t.0* @julia_call19(%jl_value_t.0* undef, %jl_value_t.0* undef, %jl_value_t.0* undef, %jl_value_t.0* undef, i64 undef, %jl_value_t.0* undef, i1 %4, i64 undef, double undef)
+  ret %jl_value_t.0* %5
+}

test/CodeGen/X86/fast-isel-x86-64.ll

 
 ; rdar://9297003 - fast isel bails out on all functions taking bools
 define void @test14(i8 %tmp) nounwind ssp noredzone {
 entry:
   %tobool = trunc i8 %tmp to i1
   call void @test13f(i1 zeroext %tobool) noredzone
   ret void
 ; CHECK-LABEL: test14:
-; CHECK: andb $1, 
+; CHECK: andl $1, 
 ; CHECK: callq
 }
 
 declare void @llvm.memcpy.p0i8.p0i8.i64(i8*, i8*, i64, i32, i1)
 
 ; rdar://9289488 - fast-isel shouldn't bail out on llvm.memcpy
 define void @test15(i8* %a, i8* %b) nounwind {
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* %a, i8* %b, i64 4, i32 4, i1 false)