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

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai added a subscriber: hfinkel.
nemanjai added a comment.

I think that [some] target maintainers should be on the list of reviewers if you hope to get this patch reviewed/committed. You can get a list of target maintainers from the `CODE_OWNERS.TXT` at the top level llvm source directory.



================
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
----------------
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.


================
Comment at: include/llvm/Target/TargetRegisterInfo.h:825
+  requiresFreezingReservedRegsBeforeRegAlloc(const MachineFunction &MF) const {
+    return false;
+  }
----------------
I am also not convinced that the default here should be to return `false`. The fact that PowerPC is the only target that has failures in the unit tests without the second call to `MachineRegisterInfo::freezeReservedRegs` could certainly be because PowerPC is the only target that needs it. But it could just as well be the case that it's just the only target that actually has a unit test that is sensitive to this.
It seems to me that the better approach would be to conservatively have both calls by default. Then allow targets that want to save on compile time and don't need both calls, to override this and not get both calls.


https://reviews.llvm.org/D29448