[PATCH] D98232: [regalloc] Ensure Query::collectInterferringVregs is called before interval iteration

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added a comment.

In D98232#2661456 <https://reviews.llvm.org/D98232#2661456>, @mtrofin wrote:

> In D98232#2661432 <https://reviews.llvm.org/D98232#2661432>, @lebedev.ri wrote:
>
>> In D98232#2661406 <https://reviews.llvm.org/D98232#2661406>, @mtrofin wrote:
>>
>>> In D98232#2660803 <https://reviews.llvm.org/D98232#2660803>, @lebedev.ri wrote:
>>>
>>>> In D98232#2660309 <https://reviews.llvm.org/D98232#2660309>, @mtrofin wrote:
>>>>
>>>>> In D98232#2657991 <https://reviews.llvm.org/D98232#2657991>, @dmgreen wrote:
>>>>>
>>>>>> Yeah, I agree. 5% is too much to pay. https://reviews.llvm.org/D69437 measured this option as a 25% speed increase in a something that was important enough to fix, with a 0.1-0.2% compile time effect. That's a very different question.
>>>>>>
>>>>>> (What really would like to see - is it would be great if there was someone in the llvm community who cared about O2 <https://reviews.llvm.org/owners/package/2/>. Trying to get the best performance for a low compile time, and optimizing that tradeoff. That can do a lot of benefit for a lot of people. This particular issue I have less interest in, It's just a shame to see the compile get things so embarrassingly wrong!)
>>>>>>
>>>>>> But this patch has been in tree a long time. I believe best practice is to fix things as-is, not revert and attempt to re-apply after so long.
>>>>>
>>>>> Quick update on the code quality side, I ran spec2006, the llvm benchmarks, the eigen benchmarks, and a few others, on x86 (FDO, thinlto) with this patch, and with/without enabling consider-local-interval-cost. No significant real effect.
>>>>>
>>>>> About how to proceed, to add to the "revert original vs not" aspect, the additional trouble is that the original patch has some changes to weight calculation that aren't controlled by neither consider-local-interval-cost, nor by enableAdvancedRASplitCost().
>>>>>
>>>>> Any pushback, then, to proceeding as planned (fixing the code, but disabling it by default everywhere)? If anything "breaks", there's a quick unblocker for those affected, and meanwhile there's time to investigate the alternatives others mentioned here.
>>>>
>>>> If it's not going to be enabled *anywhere*, even on the AArch64, it sounds like dead code to me, which shouldn't be there.
>>>
>>> Not enabled by default. This would still give folks a chance to unblock if they hit regressions by using the flag. We can then remove the portion that's flag controlled when we have the alternative others were mention, or when we feel that, since no one reported regressions, it's safe to remove.
>>
>> I understand that. The question i'm asking is whether it's useful to have that flag in the first place?
>> Do we know that people will actually use it?
>
> Ah, I see - we don't know, since right now people don't know if they'd have a regression if we flipped the default behavior. If they do, they'd easily unblock themselves, and hopefully report back. The alternative - removing the code outright - is harder to revert.

Ah, i see.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D98232/new/

https://reviews.llvm.org/D98232