[PATCH] D122615: Early return in constrainOperandRegClass()

[Viacheslav Nikolaev via Phabricator via llvm-commits]

wvoquine marked an inline comment as not done.
wvoquine added inline comments.


================
Comment at: llvm/lib/CodeGen/GlobalISel/Utils.cpp:61-65
+  // Check if the register class is already as required.
+  auto *OldRegClass = MRI.getRegClassOrNull(Reg);
+  if (OldRegClass == &RegClass)
+    return Reg;
+
----------------
wvoquine wrote:
> dsanders wrote:
> > 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
> >     }
> >   }
> > ```
> > 
> > 
> 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.
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.


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

https://reviews.llvm.org/D122615