[PATCH] D12293: Rework of the interface to enable shrink wrapping

[Quentin Colombet via llvm-commits]

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

Hi Kit,

Small nitpicks inlined but LGTM otherwise.

Thanks,
-Quentin


================
Comment at: include/llvm/Target/TargetFrameLowering.h:144
@@ -143,1 +143,3 @@
 
+  /// targetHandlesShrinkWrapping - Returns true if the target will correctly
+  /// handle shrink wrapping.
----------------
Do not repeat the method name in the comment.

================
Comment at: include/llvm/Target/TargetFrameLowering.h:146
@@ +145,3 @@
+  /// handle shrink wrapping.
+  virtual bool targetSupportsShrinkWrapping(const MachineFunction &MF) const {
+    return false;
----------------
Maybe enableShrinkWrapping?
“Supports" is kind of misleading because for instance X86 supports it, but it does not enabled it by default yet.

(Personally, I would not repeat target in the method name, but I see that are precedence :).)

================
Comment at: lib/CodeGen/ShrinkWrap.cpp:155
@@ -150,1 +154,3 @@
 
+  /// \brief Check if shrink wrapping is enabled for this target and function
+  bool isShrinkWrapEnabled(const MachineFunction &MF) const;
----------------
Period at the end of the comment.

================
Comment at: lib/CodeGen/ShrinkWrap.cpp:156
@@ +155,3 @@
+  /// \brief Check if shrink wrapping is enabled for this target and function
+  bool isShrinkWrapEnabled(const MachineFunction &MF) const;
+  
----------------
Could be a static method.

================
Comment at: lib/CodeGen/ShrinkWrap.cpp:332
@@ +331,3 @@
+
+  if (! isShrinkWrapEnabled(MF))
+    return false;
----------------
The formatting seems weird here.

================
Comment at: lib/CodeGen/ShrinkWrap.cpp:332
@@ +331,3 @@
+
+  if (! isShrinkWrapEnabled(MF))
+    return false;
----------------
qcolombet wrote:
> The formatting seems weird here.
Maybe put this check with the previous condition, i.e., if MF.empty || !isShrinkWrapEnabled(MF).


http://reviews.llvm.org/D12293