[PATCH] D33562: MachineLICM: Add new condition for hoisting of caller preserved registers

[Lei Huang via Phabricator via llvm-commits]

lei added inline comments.


================
Comment at: include/llvm/CodeGen/MachineRegisterInfo.h:584-586
+  /// Returns true if PhysReg is always preserved by the caller.
+  bool isCallerPreservedPhysReg(unsigned PhysReg) const;
+
----------------
MatzeB wrote:
> No need to add this to MachineRegisterInfo.
k


================
Comment at: include/llvm/Target/TargetRegisterInfo.h:500-501
 
+  /// Returns true if PhysReg is always caller saved.  
+  /// Used by MachineRegisterInfo::isCallerPreservedPhysReg().
+  virtual bool isCallerPreservedPhysReg(unsigned PhysReg,
----------------
MatzeB wrote:
> Please improve upon the description. Currently this sounds like a check where PhysReg is a CSR register, which you just explained to me in the review is not the case!
k


================
Comment at: test/CodeGen/PowerPC/licm-tocReg.ll:1
+; RUN: llc -verify-machineinstrs -mtriple=powerpc64le-unknown-linux-gnu < %s | FileCheck %s
+
----------------
MatzeB wrote:
> - Can you write a .mir test that is better targetted at the problem?
> - Please add a sentence or two to describe what you are testing.
When I tried to create a .mir test I get machine verifier errors.  Since there are no passes invoked that identify the TOC access in the function, we don't reserve X2.
The problem is that MachineFunctionInfo isn't saved/dumped as part of emitting .mir.  So any passes that rely on a previously populated MFI run the risk of failing.  Specifically, this is an issue with TOC access which is what I'm trying to test.  The proper solution would be to emit MFI data .mir, but that is beyond the scope of this patch.  

Would an IR test with -stop-before/-stop-after checks be a reasonable compromise?


https://reviews.llvm.org/D33562