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

[Jim Grosbach]

Nice detective work. Thanks for digging into this!

-Jim

On Jul 18, 2012, at 3:38 AM, Chandler Carruth <chandlerc at gmail.com> wrote:

> 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
> <pr13378.patch>_______________________________________________
> llvm-commits mailing list
> llvm-commits at cs.uiuc.edu
> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20120718/da6ee4a1/attachment.html>