[llvm] r280783 - Remove unnecessary call to getAllocatableRegClass

[Justin Bogner via llvm-commits]

Matt Arsenault <arsenm2 at gmail.com> writes:
>> On Sep 21, 2016, at 16:49, Justin Bogner via llvm-commits
>> <llvm-commits at lists.llvm.org> wrote:
>> 
>> Matt Arsenault via llvm-commits <llvm-commits at lists.llvm.org> writes:
>>> Author: arsenm
>>> Date: Wed Sep  7 01:16:45 2016
>>> New Revision: 280783
>>> 
>>> URL: http://llvm.org/viewvc/llvm-project?rev=280783&view=rev
>>> Log:
>>> Remove unnecessary call to getAllocatableRegClass
>>> 
>>> This reapplies r252565 and r252674, effectively reverting r252956.
>>> 
>>> This allows VS_32/VS_64 to be unallocatable like they should be.
>>> 
>>> Modified:
>>>    llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
>>>    llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.h
>>>    llvm/trunk/lib/Target/AMDGPU/SIRegisterInfo.td
>>> 
>>> Modified: llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp
>>> URL:
>>> http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp?rev=280783&r1=280782&r2=280783&view=diff
>>> ==============================================================================
>>> --- llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp (original)
>>> +++ llvm/trunk/lib/CodeGen/SelectionDAG/InstrEmitter.cpp Wed Sep  7 01:16:45 2016
>>> @@ -330,16 +330,22 @@ InstrEmitter::AddRegisterOperand(Machine
>>>   // shrink VReg's register class within reason.  For example, if VReg == GR32
>>>   // and II requires a GR32_NOSP, just constrain VReg to GR32_NOSP.
>>>   if (II) {
>>> -    const TargetRegisterClass *DstRC = nullptr;
>>> +    const TargetRegisterClass *OpRC = nullptr;
>>>     if (IIOpNum < II->getNumOperands())
>>> -      DstRC = TRI->getAllocatableClass(TII->getRegClass(*II,IIOpNum,TRI,*MF));
>>> -    assert((!DstRC || TargetRegisterInfo::isVirtualRegister(VReg)) &&
>>> -           "Expected VReg");
>>> -    if (DstRC && !MRI->constrainRegClass(VReg, DstRC, MinRCSize)) {
>>> -      unsigned NewVReg = MRI->createVirtualRegister(DstRC);
>>> -      BuildMI(*MBB, InsertPos, Op.getNode()->getDebugLoc(),
>>> -              TII->get(TargetOpcode::COPY), NewVReg).addReg(VReg);
>>> -      VReg = NewVReg;
>>> +      OpRC = TII->getRegClass(*II, IIOpNum, TRI, *MF);
>>> +
>>> +    if (OpRC) {
>>> +      const TargetRegisterClass *ConstrainedRC
>>> +        = MRI->constrainRegClass(VReg, OpRC, MinRCSize);
>>> +      if (!ConstrainedRC) {
>>> +        unsigned NewVReg = MRI->createVirtualRegister(OpRC);
>> 
>> I'm seeing an assert when createVirtualRegister is called here in one of
>> my backends, since OpRC isn't allocatable (and createVirtualRegister
>> asserts that it is).
>> 
>> Interestingly, in the review of the revert, r252956, Quentin's example
>> code handled this as I would expect:
>> 
>> In
>> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20151109/312603.html,
>> Quentin Colombet <qcolombet at apple.com> writes:
>>>> The assertion was supposed to be in the else part, not the if part.
>>>> I.e.,
>>>> if (DstRC) { // Break this if
>>>>  if (!MRI->constrainRegClass(VReg, DstRC, MinRCSize)) {
>>>>    // This is point #2 I missed.
>>>>    DstRC = TRI->getAllocatableClass(DstRC); // Refine for allocatable class.
>>>>    assert(DstRC && “Operation constraints cannot be fulfilled for allocation”);
>>>>    unsigned NewVReg = MRI->createVirtualRegister(DstRC);
>>>>    ...
>>>>  } else // add this else
>>>>      assert(OpRC->isAllocatable() &&
>>>>             "Constraining an allocatable VReg produced an unallocatable class?”);
>>>> }
>> 
>> Am I missing something here?
>
> Is this the noreg case?
>
> This code was slightly off because the returned register class of
> constrainRegClass needs to be used, so that it didn’t solve the
> original problem.

The assert I'm seeing is in the case where `OpRC != nullptr` but
`ConstrainedRC == nullptr`. Quentin's suggestion would call
getAllocatableClass(DstRC) in this case, whereas in the committed code
we just go ahead and do MRI->createVirtualRegister(OpRC). The call to
createVirtualRegister asserts because OpRC isn't allocatable.