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

Thu Feb 9 12:20:33 PST 2017

qcolombet 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
----------------
nemanjai wrote:
> 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.
Do you know what is missing to make that possible?

https://reviews.llvm.org/D29448