[llvm-commits] PATCH: Fix for PR13378 (I think...), and a lot of questions about how 2addr pass works. =]

[Chandler Carruth]

So, moving from http://llvm.org/PR13378 to here because bugs don't go to
the mailing lists, and I have a patch ready for this one. =D

Also, some others might be curious what it looks like to take a nose first
tive through this pass and the MI parts of the backend. =]

(In reply to comment #1)
> RescheduleMIBelowKill is moving an INSERT_SUBREG instruction, and then it
never
> gets revisited, so we don't insert the required COPY in front of it.

So, you piqued my curiosity, and I dug. ;] I think I have a clear
understanding of what happens, and what is wrong about the end result, and
how to fix it. However, I'm new to this code, so I'll explain in (perhaps
way more detail than you wanted) how I got here. Hopefully, you can stop me
if I've missed anything or gotten stuff wrong.

When we process this with the twoaddr pass, we eventually try to reschedule
the INSERT_SUBREG below a kill. When we start, the MBB looks like this:

BB#1: derived from LLVM BB %3
    Predecessors according to CFG: BB#0
        %vreg2<def> = VMOVv4i32 0, pred:14, pred:%noreg; QPR:%vreg2
        %vreg3<def> = VLDMQIA %vreg4<undef>, pred:14, pred:%noreg;
mem:LD16[undef](align=8) QPR_VFP2:%vreg3 GPR:%vreg4
        %vreg5<def> = FCONSTS 112, pred:14, pred:%noreg; SPR:%vreg5
        VSTMQIA %vreg2<kill>, %vreg7<undef>, pred:14, pred:%noreg;
mem:ST16[undef](align=8) QPR:%vreg2 GPR:%vreg7
        %vreg8<def> = INSERT_SUBREG %vreg3, %vreg5<kill>, ssub_3;
QPR_VFP2:%vreg8,%vreg3 SPR:%vreg5
        VSTMQIA %vreg3<kill>, %vreg9<undef>, pred:14, pred:%noreg;
mem:ST16[undef](align=8) QPR_VFP2:%vreg3 GPR:%vreg9
        VSTMQIA %vreg8<kill>, %vreg10<undef>, pred:14, pred:%noreg;
mem:ST16[undef](align=8) QPR_VFP2:%vreg8 GPR:%vreg10


This succeeds at rescheduling the INSERT_SUBREG below vreg3's kill. Which
seems a nice, reasonable thing to do. The result is:

BB#1: derived from LLVM BB %3
    Predecessors according to CFG: BB#0
        %vreg2<def> = VMOVv4i32 0, pred:14, pred:%noreg; QPR:%vreg2
        %vreg3<def> = VLDMQIA %vreg4<undef>, pred:14, pred:%noreg;
mem:LD16[undef](align=8) QPR_VFP2:%vreg3 GPR:%vreg4
        %vreg5<def> = FCONSTS 112, pred:14, pred:%noreg; SPR:%vreg5
        VSTMQIA %vreg2<kill>, %vreg7<undef>, pred:14, pred:%noreg;
mem:ST16[undef](align=8) QPR:%vreg2 GPR:%vreg7
        VSTMQIA %vreg3, %vreg9<undef>, pred:14, pred:%noreg;
mem:ST16[undef](align=8) QPR_VFP2:%vreg3 GPR:%vreg9
        %vreg8<def> = INSERT_SUBREG %vreg3<kill>, %vreg5<kill>, ssub_3;
QPR_VFP2:%vreg8,%vreg3 SPR:%vreg5
        VSTMQIA %vreg8<kill>, %vreg10<undef>, pred:14, pred:%noreg;
mem:ST16[undef](align=8) QPR_VFP2:%vreg8 GPR:%vreg10

As far as I can tell, at this point, everything is correct, and happy.

We then carry on and *do* hit the section which rewrites INSERT_SUBREG as a
COPY node around line 1607 of TwoAddressInstructionPass.cpp. So, when we
start this, we have the following INSERT_SUBREG:

  %vreg8<def> = INSERT_SUBREG %vreg3<kill>, %vreg5<kill>, ssub_3;
QPR_VFP2:%vreg8,%vreg3 SPR:%vreg5

and we rewrite it to:

  %vreg8:ssub_3<def> = COPY %vreg5<kill>; QPR_VFP2:%vreg8 SPR:%vreg5

But I don't see how this could be a correct transform. %vreg3 had some
value coming into the INSERT_SUBREG, and we're supposed to be copying that
value into %vreg8, and then coping %vreg5 over top of the subreg indexed by
'ssub_3' (18 in this case).

Note that we leave %vreg8 as <def>'ed by this operation! This in turn seems
to quite reasonably lead to the assertion as we have an instruction that
defs a vreg and also reads from it. Worse, it's *not* reading from the vreg
with the actual values, %vreg3.



Ok, so what should the fix be?

You mentioned that we don't insert the required COPY in front of it. I
assume you're referring to the COPY %vreg3 to %vreg8, giving a proper
<def>, and then allow the subreg COPY to clobber the subreg in question?
Yes, reading the tied operands processing section, that is the point of the
meat of this loop -- to inject copies (dropping SSA form) and then allow
operations to occur in-place. It all makes sense now!

Hmm, so *normally*, what happens when we reschedule this is that we leave
the "next" iterator pointing at the next instruction, and since we
rescheduled *down* the MBB, we will re-process that instruction! This
second time around, we'll be able to insert the needed copy.

This in turn leads me to the bug. When we transform the instruction by
rescheduling, we break out of the loop over tied operands on line 1473
claiming that they have been eliminated. That's a bit of a lie, they've
been either eliminated, or pushed further down in the basic block where we
can re-evaluate them. That break drops us to line 1606, which is in the
same cycle of the loop!!! Here is the bug. Now 'mi' still points to the
INSERT_SUBREG, but we haven't actually processed it into two-address form
yet, and we have fixed up the loop so that we'll reprocess it again later.
Because of this, the rewrite occurs when everything is not set up properly.

Amusingly, we *do* re-process this instruction. But by then, it's a COPY,
and doesn't even refer to the source register. Doh!

OK, so the fix is logically straightforward: the break on line 1473 really
needs to set 'mi' to 'nmi', and continue the for loop one layer out. But
that is a bit hard to actually write in the current formation. I've
attached a patch that I think just does it. =] It's gross, but the gross is
the same gross as the rest of this function. It just needs massive
refactoring. If you like this fix, I'll add the test case into the test
suite, and commit.

Let me know if you'd like me to break out a big refactoring axe and start
slicing this thing into a separate functions. I could probably spare a few
hours to just make no-functionality changed refactoring patches and get
this into at least a somewhat cleaner state.


Thanks for bearing with my lengthy wandering in the email. =] Hopefully the
patch helps offset it some.
-Chandler
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120718/cc76426c/attachment.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: pr13378.patch
Type: application/octet-stream
Size: 3621 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120718/cc76426c/attachment.obj>