Page MenuHomePhabricator

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

Authored by loladiro on Nov 13 2014, 1:04 AM.

Details

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.

Diff Detail

Repository
rL LLVM

Event Timeline

loladiro updated this revision to Diff 16138.Nov 13 2014, 1:04 AM
loladiro retitled this revision from to [FastIsel][X86] Fix invalid register replacement for bool args.
loladiro updated this object.
loladiro edited the test plan for this revision. (Show Details)
loladiro added a reviewer: ributzka.
loladiro set the repository for this revision to rL LLVM.
loladiro added a subscriber: Unknown Object (MLST).
ributzka edited edge metadata.Nov 13 2014, 9:26 AM

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 updated this revision to Diff 16176.Nov 13 2014, 1:36 PM
loladiro updated this object.
loladiro edited edge metadata.
loladiro set the repository for this revision to rL LLVM.

Address review comments

ributzka accepted this revision.Nov 13 2014, 1:42 PM
ributzka edited edge metadata.

Great. Thanks for fixing this.

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

LGTM.

This revision is now accepted and ready to land.Nov 13 2014, 1:42 PM

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

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

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

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

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 planned changes to this revision.Nov 13 2014, 6:42 PM
loladiro updated this revision to Diff 16189.Nov 13 2014, 6:59 PM
loladiro edited edge metadata.

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?

This revision is now accepted and ready to land.Nov 13 2014, 6:59 PM

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

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

This revision was automatically updated to reflect the committed changes.