[PATCH] D69835: Add options for PPC to enable/disable using non-volatile CR

[Nemanja Ivanovic via Phabricator via llvm-commits]

nemanjai accepted this revision.
nemanjai added a comment.
This revision is now accepted and ready to land.

The remaining comments are stylistic nits that can be addressed on the commit. Assuming that those will be fixed, LGTM.



================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.cpp:18
+PPCDisableNonVolatileCR("ppc-disable-non-volatile-cr",
+                        cl::desc("Disable non-volatile CR/CR bit"),
+                        cl::init(false), cl::Hidden);
----------------
"Disable the use of non-volatile CR register fields."


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h:68
 
+  /// DisableNonVolatileCR - Inidcates whether non-volatile CR fields would be disabled
+  bool DisableNonVolatileCR = false;
----------------
Line too long?


================
Comment at: llvm/lib/Target/PowerPC/PPCMachineFunctionInfo.h:181
 
+  void setNoNonVolatileCR()    { DisableNonVolatileCR = true; }
+  bool isNonVolatileCRDisabled() const { return DisableNonVolatileCR; }
----------------
The naming is odd here - you use `No` for the setter and `Disabled` for the getter. Please name them consistently. Perhaps `setDisableNonVolatileCR()` and `isNonVolatileCRDisabled()`.


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.td:366
   let Size = 32;
+  let AltOrders = [(sub CRBITRC, CR2LT, CR2GT, CR2EQ, CR2UN, CR3LT, CR3GT, CR3EQ, CR3UN, CR4LT, CR4GT, CR4EQ, CR4UN)];
+  let AltOrderSelect = [{
----------------
Please keep to 80 columns in .td files as well.


================
Comment at: llvm/lib/Target/PowerPC/PPCRegisterInfo.td:369
+    const PPCFunctionInfo *FI = MF.getInfo<PPCFunctionInfo>();
+    return MF.getSubtarget<PPCSubtarget>().isELFv2ABI() && FI->isNonVolatileCRDisabled();
+  }];
----------------
Yi-Hong.Lyu wrote:
> Why do you NOT introduce a temp variable in the former condition but introduce one in the later one? I would suggest choose one style and use that uniformly.
+1


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

https://reviews.llvm.org/D69835