[PATCH] D17948: Use CXX_FAST_TLS to enable shrink wrapping on PPC

[Kit Barton via llvm-commits]

kbarton requested changes to this revision.
kbarton added a comment.
This revision now requires changes to proceed.

A couple general comments:

1. Can we add statistics to collect the number of times we've made changes? I think this would be useful to understand how effective this is.
2. In general, I think we should prefer references to pointers unless there is a chance the parameter can be null. I think most of the cases here could be references. If they need to stay as pointers, please make sure we check that they are valid before using them.
3. We should also mark the parameters as const whenever they are not expected to change. I believe that is true for many of the methods here, as they are just checking for a property and then returning true/false.


================
Comment at: lib/Target/PowerPC/PPCEnableShrinkWrap.cpp:68
@@ +67,3 @@
+
+  static bool branchesTo(TerminatorInst *Term, BasicBlock *Succ) {
+    for (unsigned i = 0; i < Term->getNumSuccessors(); ++i)
----------------
These could both be reference parameters, instead of pointers.
Can they be made const as well?

================
Comment at: lib/Target/PowerPC/PPCEnableShrinkWrap.cpp:115
@@ +114,3 @@
+
+  static bool canBeComputedWithoutStack(BasicBlock *exitBlock,
+                                        BasicBlock &entry) {
----------------
Could this be a reference instead of a pointer?

================
Comment at: lib/Target/PowerPC/PPCEnableShrinkWrap.cpp:128
@@ +127,3 @@
+
+  static bool hasCallInst(Function &F) {
+    for (auto &BB : F)
----------------
Can F be const?

================
Comment at: lib/Target/PowerPC/PPCEnableShrinkWrap.cpp:145
@@ +144,3 @@
+
+  static bool mayHaveSideEffects(BasicBlock &BB) {
+    for (auto &I : BB)
----------------
Can BB be const?


http://reviews.llvm.org/D17948