[PATCH] D22718: MachineFunction: Remove AllVRegsAllocated property

Tue Aug 9 11:39:43 PDT 2016

> On Aug 9, 2016, at 11:11 AM, Matthias Braun <matze at braunis.de> wrote:
> 
>> 
>> On Aug 9, 2016, at 10:03 AM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>> 
>>> 
>>> On Aug 8, 2016, at 6:06 PM, Matthias Braun <matze at braunis.de <mailto:matze at braunis.de>> wrote:
>>> 
>>>> 
>>>> On Aug 8, 2016, at 5:53 PM, Quentin Colombet via llvm-commits <llvm-commits at lists.llvm.org <mailto:llvm-commits at lists.llvm.org>> wrote:
>>>> 
>>>> Hi Matthias,
>>>> 
>>>> I haven’t read all the discussion but I think I read enough to chime in.
>>>> I would like to keep the AllVRegsAllocated property.
>>>> 
>>>> I’m fine that the parser recomputes it based on the number of virtual registers, but I do not want it to be tight to that information dynamically. I believe part of the problem is in the naming of the property and the semantic attached to it.
>>>> This property means that:
>>>> 1. No virtual registers are present (the dynamic check capture that)
>>>> 2. Passes cannot introduce new VRegs (the name does not convey that, but the dynamic check would lose that information)
>>>> 
>>>> Regarding your example:
>>>> file1.mir:
>>>> ...
>>>> name: foo
>>>> AllVRegsAllocated: true
>>>> body: |
>>>>  bb.0:
>>>>    RETQ
>>>> ---
>>>> 
>>>> 
>>>> 
>>>> file2.mir
>>>> ...
>>>> name: foo
>>>> AllVRegsAllocated: false
>>>> body: |
>>>>  bb.0:
>>>>    RETQ
>>>> —
>>>> 
>>>> This are semantically different in the sense that a pass working on that input could performance differently based on the value of the property. E.g., with AllVRegsAllocated == false, it could introduce vregs, whereas with AllVRegsAllocated == true, it would run the scavenger before exiting. If we make the property dynamic, the pass won’t see it has to scavenge the vregs it introduced.
>>> But if we imagine such a situation (today we don't AFAIK) with pass "FooBar", then wouldn't it be simpler to have "FooBarBeforeRegAlloc" and "FooBarAfterRegAlloc" and push that decision on the code setting up the pass pipeline instead of the "FooBar" pass switching modes based on a property in the MI?
>> 
>> That already happens with passes like MachineLICM that performs differently before or after regalloc. Sure we could have a different mode for the pass that we set in the pipeline, though I like the property in particular for the MachineVerifier that gets added automatically. We may change how it is added so that it knows where it is in the pipeline, but as we add more properties it does not scale.
> I don't see why this wouldn't scale. You can always add more parameters to the pass constructor if you want to avoid creating complete new subclasses for different situations.

That’s the problem, you then get more and more parameters for different properties. Like isAfterLegalization, isAfterRegBankSelect, isAfterISel.
The properties abstract us away and the passes do what they want.
I kind of like the idea of having that being parameters but where it does not scale is for the MachineVerifier. Again that thing is added automatically and feeding the proper parameters to the pass seems complicated in the current scheme.

> 
> In fact I think we should change MachineLICM to not rely on the isSSA property to detect whether it runs before or after regalloc. We should rather have a switch in the constructor that configures that.
> 
>> 
>>> The later style can already be seen in the MachineScheduler/PostMachineScheduler which both use the same base class but are specialized into two independent passes.
>>> 
>>>> 
>>>> Again, I believe this is a naming problem, but I don’t have a great naming, CanUseVRegs maybe?
>>> It's not even "CanUseVRegs" because we actually can and do use: Most frame lowering code creates temporary vregs (that are replaced by physregs in PrologueEpilogueInserter) so all this means is "NoVRegs between passes" (and for that see above for splitting into different passes).
>> 
>> That’s what I meant. I am fine with such name.
>> 
>>> 
>>>> 
>>>> Also, staying on that example, what would be the default value in that case?
>>> Again I believe this to be a property of scheduling passes correctly not a property of the MachineIR. If there is no semantic difference in the two functions above then it would be best if they are the same.
>>> If anything we would need this to be a property of the passes and have some pass verification layer on top that checks the correct order of the passes. However I personally find the assert()s strong enough to catch typical errors here so we do not need extra machinery…
>> 
>> We already have this “extra” machinery… I don’t see what you do not like with that.
>> Also if you go in that direction, then isSSA shouldn’t be a property as well we can recompute it as well, but again I believe those are two different goal and I do like the property machinery.
> 
> This is exactly why I have another patch out there that does not print or parse isSSA in .mir files but computes it instead.
> I think of isSSA as a cached analysis result. I can always recompute the information but checking the bool field and keeping it up to date is faster than checking all vregs for the SSA property in multiple time throughout codegen.

Same here, even if you get isSSA == yes, that does not mean you want to preserve that. E.g., if the function is just a BB and it happens to be in SSA.

> 
> I can live with the machinery. What I find annoying/wrong with CanUseVRegs is that we have a property in the MI that has no semantic meaning at all, it seems to be purely a configuration setting for the passes we run on a given MI function.

At least, it has a semantic for me :).

> 
> - Matthias

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