[PATCH] D29448: [RegAlloc] Don’t freeze reserved registers again if it is not necessary

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added inline comments.


================
Comment at: include/llvm/Target/TargetRegisterInfo.h:822
+  /// Returns true if the target wants to update the reserved register
+  /// before the register allocation process.
+  virtual bool
----------------
qcolombet wrote:
> nemanjai wrote:
> > I think a comment with an example of why this may be needed by a target is in order here. For example, PPC requires it because it has a different set of reserved registers if a function ends up having a `TargetOpcode::PATCHPOINT` node in it.
> > Perhaps @hfinkel has a different suggestion here.
> My understanding is that this kind of things should be settled at ISel time. I.e., I would expect only one call to the freeze function per function. If we need more, I'd say our representation is broken.
> 
> I'll dig a little more to make sure we are not missing anything (in particular I'll have a look at r168630), but ultimately, I would suggest that all targets work with just one call to this.
The relevant comment from the PPC back end at the location where the missing reserved register is reconciled is in PPCISelLowering.cpp and reads:
```
      // Call lowering should have added an r2 operand to indicate a dependence
      // on the TOC base pointer value. It can't however, because there is no
      // way to mark the dependence as implicit there, and so the stackmap code
      // will confuse it with a regular operand. Instead, add the dependence
      // here.
```

My initial comment was that we shouldn't have a target query for this but should just resolve whatever is preventing PPC from doing the right thing at ISEL time. But that comment seemed made me change my mind.
If we can get that fixed in the PPC back end, I'm in favour of that solution over a target query.


https://reviews.llvm.org/D29448