Check if register class was changed in constrainOperandRegClass()

[wvoquine]

Summary

NFC
When no actual change happens there's no need to notify the
observers about the fact the register class is being constrained.
So we should avoid notifying observers when no change has
happened, because this can dramatically affect compile
time for particular test cases.

[dsanders]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

61–65

I'm not keen on duplicating internals of constrainRegToClass() behaviour (even obvious behaviours like this one). Do you get a similar perf fix if you check between the possible change and the notification with something like this?:

auto *OldRegClass = MRI.getRegClassOrNull(Reg);
Register ConstrainedReg = constrainRegToClass(MRI, TII, RBI, Reg, RegClass);
if (ConstrainedReg != Reg) {
  insert copy
  notify observers
} else {
  auto *NewRegClass = MRI.getRegClassOrNull(Reg);
  if (OldRegClass != &NewRegClass) {
    notify observers
  }
}

97

It's not for this patch but this call is supposed to happen before the change. It won't work properly for all observers if it's after the change. For example, the debug location info checking one will compare the state after the change to the state after the change with this call where it currently is.

[wvoquine]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

61–65

We discussed that issue with Daniel separately. It looks like a viable approach could be to add API calls RBI.needToConstrainGenericRegister()->MRI. needToConstrainGenericRegister()->...
And the implementation of those calls would be involved in the corresponding check within chain of calls of constrainRegToClass() - so that we would be checking precisely the same way as the function imposing the constraints.

[arsenm]

llvm/include/llvm/CodeGen/RegisterBankInfo.h

644 ↗

(On Diff #418748)

I don't see the point of having this be both a separate utility function and a static member. It should be either one or the other

llvm/lib/CodeGen/GlobalISel/Utils.cpp

46

Why call a static method through RBI?

[wvoquine]

llvm/include/llvm/CodeGen/RegisterBankInfo.h

644 ↗

(On Diff #418748)

I just followed the choices made for the corresponding utility functions/methods for constrainGenericRegister() and constrainOperandRegClass().

There's no actual need for the latter to be around at the moment, I can remove the utility function.

llvm/lib/CodeGen/GlobalISel/Utils.cpp

46

Good point!
By the way, the same would apply to the llvm::constrainRegToClass() implementation below... I just replicated that sort of code without noticing it was actually a static method.

I guess, it really makes sense to remove this utility method right now, to avoid fixing the API some of the operands of which are not required...

[wvoquine]

llvm/include/llvm/CodeGen/RegisterBankInfo.h

644 ↗

(On Diff #418748)

There's no actual need for the latter

I mean, there's no real need for needToConstrainRegToClass() at the moment.

[dsanders]

llvm/lib/CodeGen/MachineRegisterInfo.cpp

82 ↗

(On Diff #418748)

This is more of a naming nitpick but this path isn't constraining the reg class yet constraining is still needed. I believe the caller is expected to emit a copy instead and that caller should notify the observers correctly

FWIW, MinNumRegs is hardly ever used, I can only find a couple instances

llvm/lib/CodeGen/RegisterBankInfo.cpp

131–132 ↗

(On Diff #418748)

There's also a chance it isn't and constrainGenericRegister() changes the class requiring that the observers be notified

[wvoquine]

llvm/lib/CodeGen/MachineRegisterInfo.cpp

82 ↗

(On Diff #418748)

Maybe I should call needsConstrainOrCopy() ?

llvm/lib/CodeGen/RegisterBankInfo.cpp

131–132 ↗

(On Diff #418748)

You mean, it's not constrained appropriately but the Reg's class is the same?

[wvoquine]

llvm/lib/CodeGen/GlobalISel/Utils.cpp

61–65

After further discussion of the new patch, it turns out the post-check @dsanders has proposed would be the right way to go. The reasons are

1. The proposed API doesn't fully solve the pre-check problem anyway, so would still be an ad hoc solution.
2. To make it less ad hoc the function needToConstrainRegToClass() would need to be saying if a register class change is actually going to happen in the constrainRegToClass() call. And I personally don't see a good name for such a function.
3. A better approach would be to pass the observers to constrainRegToClass() but that would be a large interface change.

So, considering all this I'm leaning towards the proposed "post-check" suggestion with a "TODO:" about a solution which would be not an ad hoc one.

[wvoquine]

Went ahead with the post-check Daniel proposed.
This is a cleaner approach - we just check whether the register class has changed as a part of the constraining and based on that decide whether we need to notify the observers.

[wvoquine]

Thanks for accepting!

Could anyone, please, submit the change?

[dsanders]

LGTM

llvm/lib/CodeGen/GlobalISel/Utils.cpp

 
 using namespace llvm;
 using namespace MIPatternMatch;
 
 Register llvm::constrainRegToClass(MachineRegisterInfo &MRI,
                                    const TargetInstrInfo &TII,
                                    const RegisterBankInfo &RBI, Register Reg,
                                    const TargetRegisterClass &RegClass) {
   if (!RBI.constrainGenericRegister(Reg, RegClass, MRI))
     return MRI.createVirtualRegister(&RegClass);
 
   return Reg;
 }
 
 Register llvm::constrainOperandRegClass(
     const MachineFunction &MF, const TargetRegisterInfo &TRI,
     MachineRegisterInfo &MRI, const TargetInstrInfo &TII,
     const RegisterBankInfo &RBI, MachineInstr &InsertPt,
     const TargetRegisterClass &RegClass, MachineOperand &RegMO) {
   Register Reg = RegMO.getReg();
   // Assume physical registers are properly constrained.
   assert(Register::isVirtualRegister(Reg) && "PhysReg not implemented");
 
+  // Save the old register class to check whether
+  // the change notifications will be required.
+  // TODO: A better approach would be to pass
+  // the observers to constrainRegToClass().
+  auto *OldRegClass = MRI.getRegClassOrNull(Reg);
   Register ConstrainedReg = constrainRegToClass(MRI, TII, RBI, Reg, RegClass);
   // If we created a new virtual register because the class is not compatible
   // then create a copy between the new and the old register.
   if (ConstrainedReg != Reg) {
     MachineBasicBlock::iterator InsertIt(&InsertPt);
     MachineBasicBlock &MBB = *InsertPt.getParent();
     // FIXME: The copy needs to have the classes constrained for its operands.
     // Use operand's regbank to get the class for old register (Reg).
if (ConstrainedReg != Reg) {
     }
     if (GISelChangeObserver *Observer = MF.getObserver()) {
       Observer->changingInstr(*RegMO.getParent());
     }
     RegMO.setReg(ConstrainedReg);
     if (GISelChangeObserver *Observer = MF.getObserver()) {
       Observer->changedInstr(*RegMO.getParent());
     }
-  } else {
+  } else if (OldRegClass != MRI.getRegClassOrNull(Reg)) {
     if (GISelChangeObserver *Observer = MF.getObserver()) {
       if (!RegMO.isDef()) {
         MachineInstr *RegDef = MRI.getVRegDef(Reg);
         Observer->changedInstr(*RegDef);
       }
       Observer->changingAllUsesOfReg(MRI, Reg);
       Observer->finishedChangingAllUsesOfReg();
     }
   }
   return ConstrainedReg;
 }
 
 Register llvm::constrainOperandRegClass(
     const MachineFunction &MF, const TargetRegisterInfo &TRI,