[PATCH] D22710: Add option to CodeExtractor for hoisting dominated static allocas

[Sean Silva via llvm-commits]

silvas added a subscriber: silvas.
silvas added a comment.

A couple nits. Will look closer tomorrow.

We don't use the date convention on test cases anymore. You can just call the test case `ExtractDominatedStaticAllocas.ll`


================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Utils/CodeExtractor.cpp:728
@@ +727,3 @@
+  // If we are extracting the dominated allocas then we need to remove them
+  //  from the inputs.
+  if (ExtractDominatedAllocas)
----------------
It's a bit unusual to have this extra space here on all the lines except the first (I noticed this in one of your other patches too).

================
Comment at: /Users/rriddle/Desktop/llvm/llvm/lib/Transforms/Utils/CodeExtractor.cpp:730
@@ +729,3 @@
+  if (ExtractDominatedAllocas)
+    for (unsigned long I = 0, E = inputs.size(); I < E; ++I) {
+      // Only take static allocas.
----------------
This can just be regular `int` or `unsigned` for the loop variable. Also, for whatever reason, the standard integer variables are `i` and `e` in LLVM even though otherwise our convention is for `CamelCase` variable names. `I` and `E` would be used for iterators however.

Also, even better, this can become a range-for loop.


https://reviews.llvm.org/D22710