[PATCH] Improve machine code correctness in codegen

Andrew Trick atrick at apple.com
Fri Oct 11 14:04:39 PDT 2013 

On Oct 11, 2013, at 11:25 AM, Andrew Trick <atrick at apple.com> wrote:

> 
> On Oct 11, 2013, at 10:16 AM, Matthias Braun <mbraun at apple.com> wrote:
> 
>> As having a utility to track liveness PostRA in an ad hoc way is something needed more often, here is a new proposal that introduces a new helper class to track liveness in an ad-hoc way (given that live-ins/outs of a block are known). Fixing/refactoring the IfConversion is then a “demonstration” of that new helper class. This updated patch also comes with a simpler testcase.
>> 
>> I’m not very proud of the regmask handling in LiveRegSet but couldn’t see any better way to do it yet.
>> 
>> Greetings,
>> 	Matthias
>> 
>> <0001-Introduce-ad-hoc-liveness-tracking-utility-LiveRegSe.patch>
>> <0002-Remove-kill-flags-after-if-conversion-if-necessary.patch>
> 
> Let's call this LiveRegUnits to clearly distinguish it from vreg liveness. (The internal set can be LiveUnitSet or something...)
> 
> I would allocate more space for the SmallSet. Maybe 32.
> 
> s/brieg/brief/
> 
> I think we need a comment on RemoveRegsInMask explaining that we assume the high bits of a physical register are not preserved unless the instruction has an implicit-use operand reading the super-register, or the target declares a register unit for the upper bits.

Looking at this more closely, I don’t see how it is safe to remove register unit liveness this way. When a register is clobbered, you’re removing liveness of all the register’s units. We need to do the opposite: remove a register unit only if all its parent registers are clobbered. Conservative liveness would simply skip this step.

-Andy

> In StepForward I think we should ignore dead defs.
> 
> Go ahead and commit after your fixes. I don't need another precommit review.
> 
> -Andy
> 
>> 
>> On Oct 10, 2013, at 7:08 PM, Andrew Trick <atrick at apple.com> wrote:
>> 
>>> 
>>> On Oct 10, 2013, at 4:38 PM, Matthias Braun <mbraun at apple.com> wrote:
>>> 
>>>> 
>>>> On Oct 7, 2013, at 1:15 PM, Andrew Trick <atrick at apple.com> wrote:
>>>> 
>>>>> 
>>>>> On Oct 7, 2013, at 12:59 PM, Matthias Braun <mbraun at apple.com> wrote:
>>>>> 
>>>>>> 
>>>>>> On Oct 6, 2013, at 10:52 PM, Andrew Trick <atrick at apple.com> wrote:
>>>>>> 
>>>>>>> Thanks for the fix Matthias. It's definitely beyond the call of duty.
>>>>>>> 
>>>>>>> In the diamond case, I don't understand why you start with false-block live-ins and walk forward. We only care about liveness generated within the false block, right? We don't care about registers live out of the diamond. Why not start with an empty live set and walk backward from the end of the false-block back to the last duplicated instruction (DI2)? Adding uses and clearing defs. No need to look at kill flags. The more we can remove dependence on kill/dead flags the better.
>>>>>> Well what I needed is the set of registers live after the last duplicated instruction, these values musn’t be killed in the true block. You are right that instead of using the live-ins and walking forward (using kill and dead flags) I could also use the live-outs and walk backwards. I’ll prepare a changed patch, maybe creating a first version of that “liveness utility”.
>>>>> 
>>>>> That would be nice, but I don’t think we care about the live outs in this case (kill flags on the false path would already be wrong).
>>>>> -Andy
>>>> 
>>>> Here’s a new version of the patch with liveness computed from the end of the block so we don’t have to rely on kill flags for the computation (too bad I didn’t have that bit ready this morning). Also note that the liveness part uses ConstMIBundleOperands instead of ConstMIOperands now.
>>>> 
>>>> Greetings and thanks for the review
>>>> 	Matthias
>>>> 
>>>> <fix-bad-kill-flags-after-if-conversion.patch>
>>> 
>>> Yep. It’s identical to the helper I’m using except mine operates on register units. This looks good to me.
>>> 
>>> -Andy
>>> 
>>>>> 
>>>>>> 
>>>>>> Greetings
>>>>>> 	Matthias
>>>>>> 
>>>>>>> 
>>>>>>> Anyway, I would really like a generic backward liveness analysis utility that works on postRA MI. This seems like the ideal place for it.
>>>>>>> 
>>>>>>> -Andy
>>>>>>> 
>>>>>>> On Oct 3, 2013, at 4:53 PM, Matthias Braun <mbraun at apple.com> wrote:
>>>>>>> 
>>>>>>>> Attach is a revised version of the if conversion patch with a test case included.
>>>>>>>> 
>>>>>>>> Description: remove kill flags after if conversion if necessary
>>>>>>>> When if converting something like:
>>>>>>>> true:
>>>>>>>>    ... = R0<kill>
>>>>>>>> 
>>>>>>>> false:
>>>>>>>>    ... = R0<kill>
>>>>>>>> 
>>>>>>>> then the instructions of the true block must not have a <kill> flag
>>>>>>>> anymore, as the instruction of the false block follow and do still read
>>>>>>>> the R0 value.
>>>>>>>> Specifically this patch determines the set of register live-in in the
>>>>>>>> false block (possibly after simulating the liveness changes of the
>>>>>>>> duplicated instructions). Each of these live-in registers mustn't be
>>>>>>>> killed.
>>>>>>>> 
>>>>>>>> Greetings	
>>>>>>>> 	Matthias
>>>>>>>> 
>>>>>>>> <remove-kill-flags-after-if-conversion-if-necessary.patch>
>>>>>>>> 
>>>>>>>> On Oct 3, 2013, at 11:02 AM, Matthias Braun <mbraun at apple.com> wrote:
>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> On Oct 3, 2013, at 10:52 AM, Eric Christopher <echristo at gmail.com> wrote:
>>>>>>>>> 
>>>>>>>>>> The standard mechanism is one patch per email. Also, usually more
>>>>>>>>>> description as well.
>>>>>>>>> I hope the descriptions in the commit-messages are enough. Here they are again:
>>>>>>>>> 
>>>>>>>>> * fix bad kill flags after if conversion:
>>>>>>>>> If the true block had kill flags on registers being used in the false
>>>>>>>>> block, then these kill flags must be removed after if conversion.
>>>>>>>>> 
>>>>>>>>> * fix post ra scheduler setting wrong kill flags in bundles
>>>>>>>>> The PostRA scheduler only looked at the BUNDLE marker instruction, which
>>>>>>>>> resulted in invalid kill flags inside the bundle. This version changes
>>>>>>>>> it so the kill flags are set inside the bundle at the latest bundle
>>>>>>>>> instruction possible.
>>>>>>>>> 
>>>>>>>>> * MachineVerifier: allow physreg use if just a subreg is defined
>>>>>>>>> We can't mark partially undefined registers, so we have to allow reading
>>>>>>>>> a register in the machine verifier if just parts of a register are
>>>>>>>>> defined.
>>>>>>>>> 
>>>>>>>>> Thanks
>>>>>>>>> 	Matthias
>>>>>>>>> 
>>>>>>>>>> 
>>>>>>>>>> -eric
>>>>>>>>>> 
>>>>>>>>>> On Thu, Oct 3, 2013 at 10:44 AM, Matthias Braun <mbraun at apple.com> wrote:
>>>>>>>>>>> The attached patches improve code correctness (in the sense of -verify-machineinstrs) in codegen. This mainly improves the situation for the arm backend. Please review, thanks.
>>>>>>>>>>> 
>>>>>>>>>>> Greetings
>>>>>>>>>>>       Matthias
>>>>>>>>>>> 
>>>>>>>>>>> 
>>>>>>>>>>> _______________________________________________
>>>>>>>>>>> llvm-commits mailing list
>>>>>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>>>>> 
>>>>>>>>> 
>>>>>>>>> _______________________________________________
>>>>>>>>> llvm-commits mailing list
>>>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>>>>>>>> 
>>>>>>>> _______________________________________________
>>>>>>>> llvm-commits mailing list
>>>>>>>> llvm-commits at cs.uiuc.edu
>>>>>>>> http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits
>> 
> 
> _______________________________________________
> 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/20131011/14a6b907/attachment.html>

More information about the llvm-commits mailing list