[PATCH] ScheduleDAGInstrs should toggle kill flags on bundled instrs

[Pete Cooper]

> On May 1, 2015, at 7:00 PM, Andrew Trick <atrick at apple.com> wrote:
> 
>> 
>> On May 1, 2015, at 5:48 PM, Pete Cooper <peter_cooper at apple.com> wrote:
>> 
>> Hi Andy
>> 
>> The attached patch (and test case) is a bug found by the machine verifier.  The problem is that prior to scheduling we had
>> 
>> %R3<def> = t2ANDrr %R0 …
>> 	BUNDLE %ITSTATE<imp-def,dead>, %R6<imp-def>, %R0<imp-use,kill>, %R4<imp-use,kill>, %CPSR<imp-use,kill>, %R6<imp-use,kill>
>> 	  * t2IT 1, 24, %ITSTATE<imp-def>
>> 	  * %R6<def,tied6> = t2ORRrr %R0<kill>, %R4<kill>, pred:1, pred:%CPSR<kill>, opt:%noreg, %R6<imp-use,kill,tied0>, %ITSTATE<imp-use,kill,internal>
>> 
>> but after scheduling we moved the AND later, and cleared the kill flag only from the imp-use on the BUNDLE, not the kill on any of the instructions inside the bundle.
>> 
>> The verifier then thinks that this bundle killed R0, and so the AND after it is reading an undefined physreg.
>> 
>> This patch updates toggleKillFlag to also toggle the kill flags of instructions inside the bundle to match.  It does this bottom up in the bundle as if we are setting a kill flag then we only want to set it on the last use.
>> 
>> The test here demonstrates the issue, although i don’t think its suitable for commit.  Constructing a test which will have these bundled instructions along with guaranteeing that the AND will be scheduled later just isn’t feasible.  I hope this is ok.
>> 
>> Let me know what you think.
> 
> I don’t have a problem with the fix. I do have a problem with the fact that a backend is still calling finalizeBundles. It’s an invitation for bugs. I thought we had successfully obsoleted the BUNDLE instruction with MIBundleOperands. Are there ARM specific passes that just need to be converted to MIBundleOperands.
You’re right that we need to make sure MIBundleOperands is used correctly everywhere.  I think this bug in particular shows that unfortunately we sometimes need to specifically walk either the BUNDLE, or the instructions inside, or both, in slightly different ways and keep them up to date.

Its very possible that this particular issue is just the verifier being overly conservative about bundles, or perhaps even missing a verifier about bundles themselves.  I doubt we should have a kill on a BUNDLE operand without a corresponding kill inside the bundle for example.
> 
> Anyway, I don’t want to hold this fix up, so LGTM, with a plea for fixing bundles the right way..
Thanks.  r236428.

Cheers,
Pete
> 
> Andy
> 
>> 
>> Cheers
>> Pete
>> 
>> <scheduler-bundle-kills.diff><opt.ll>

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