[PATCH] D11649: [regalloc] Make RegMask clobbers prevent merging vreg's into PhysRegs when hoisting def's upwards.

[Hans Wennborg]

On Fri, Jul 31, 2015 at 1:24 PM, Daniel Sanders
<Daniel.Sanders at imgtec.com> wrote:
> Thanks.
>
> Because we're in rc2 now and the rules are stricter: Hans: Is this ok with you too?

Yes, go ahead.

Thanks,
Hans


> ________________________________________
> From: Quentin Colombet [qcolombet at apple.com]
> Sent: 31 July 2015 17:59
> To: reviews+D11649+public+c6823873e6bbbad6 at reviews.llvm.org
> Cc: Daniel Sanders; matze at braunis.de; hans at chromium.org; llvm-commits at cs.uiuc.edu
> Subject: Re: [PATCH] D11649: [regalloc] Make RegMask clobbers prevent merging vreg's into PhysRegs when hoisting def's upwards.
>
>> On Jul 31, 2015, at 6:08 AM, Daniel Sanders <Daniel.Sanders at imgtec.com> wrote:
>>
>> dsanders added a comment.
>>
>> I've used the earlier LGTM to commit so that I have something to merge to the release branch. I will of course follow up on the comment I received after the LGTM.
>>
>> I fixed a silly mistake before the commit. The inner if had two statements and no '{ }' so the return false was always executed. The mistake showed up as failures in four PowerPC tests.
>>
>> Ok to merge to the branch?
>
> Yep, OK to merge to the branch!
>
> Q.
>
>>
>>
>> ================
>> Comment at: lib/CodeGen/RegisterCoalescer.cpp:1541
>> @@ -1534,1 +1540,3 @@
>> +          return false;
>> +      }
>>     }
>> ----------------
>> qcolombet wrote:
>>> MatzeB wrote:
>>>> dsanders wrote:
>>>>> qcolombet wrote:
>>>>>> This check should have been handled by the loop starting line 1497, IIRC.
>>>>>> I.e., this shouldn’t be necessary.
>>>>> I agree that that loop should have caught this but it doesn't. I haven't looked into why that loop doesn't catch it yet but if the information is based on MachineInstr::definesRegister() then it could be that that doesn't seem to account for regmasks.
>>>>>
>>>>> I also tried:
>>>>>    if (MI->definesRegister(DstReg, TRI)) {
>>>>>      DEBUG(dbgs() << "\t\tInterference (read): " << *MI);
>>>>>      return false;
>>>>>    }
>>>>> in this loop but that doesn't catch it either.
>>>> From what I can see we do not create mini liverange segments for clobbers. This is both a bit scary but also understandable from a performance point of view as I imagine creating countless small segments would increase the LiveIntervals quite a bit.
>>>>
>>>> The register allocators (the LiveRegMatrix class) has a special code paths to deal with regmasks.
>>>>
>>>> That definesRegister() doesn't account for regmasks is understandable as the register isn't defined to something sensible, but there is certainly a case to make for MachineInstr::killsRegister() to check for register masks.
>>> I guess Matthias is right about the small segments.
>>>
>>> Then, I would suggest a slightly different API to use.
>>> Use the analyzePhysReg on the MachineOperand iterator.
>>> This will tell you if the physreg is read, write, or clobbered, and you can get rid of the previous loop to do the interference check.
>> I've looked at that function and I think it will do the right thing. I'll post another patch to make this change shortly.
>>
>>
>> http://reviews.llvm.org/D11649
>>
>>
>>
>