[PATCH] D22718: MachineFunction: Remove AllVRegsAllocated property

[Matthias Braun via llvm-commits]

> On Aug 8, 2016, at 5:53 PM, Quentin Colombet via llvm-commits <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?
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).

> 
> 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...

- Matthias

> 
> Cheers,
> -Quentin  
> 
>> On Jul 25, 2016, at 1:53 PM, Krzysztof Parzyszek <kparzysz at codeaurora.org> wrote:
>> 
>> kparzysz added a comment.
>> 
>> Yes, it is strange.  Whether the RA has already run or not is not really a state of the function, I agree with that.  I'm not opposed to this change, I'm just bringing up a potential consideration, that's all.
>> 
>> 
>> Repository:
>> rL LLVM
>> 
>> https://reviews.llvm.org/D22718
>> 
>> 
>> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits