[PATCH] D37902: [CodeExtractor] Fix multiple bugs under certain shape of extracted region

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added a comment.

In https://reviews.llvm.org/D37902#878726, @myhsu wrote:

> @chandlerc 
>  Temporarily revert to version before applying clang-format


I think the point was to only format your changes, not the surrounding code. You can use the `clang-format-diff.py` script to to that.



================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:785
+      new StoreInst(outputs[i], &*OAI, InsertPt);
+      ++OAI;
+    }
----------------
Why is it the case that incrementing OAI works here and gets to OutputArgEnd? Same with the GEP::Create, why is passing the OAI pointer there valid if it doesn't get incremented and you use `outputs[i]` the same time?

This is not clear to me, could you consider adding a comment?
I think that the comment from the previous version of this snippet got lost here.


================
Comment at: unittests/Transforms/Utils/CodeExtractor.cpp:56
+  }
+  // CodeExtractor require first element
+  // should dominate others
----------------
nit: s/require/requires
Maybe it'd be better to phrase is like `CodeExtractor requires the first element to dominate all the other ones`?. Isn't an `element` really a BasicBlock here?


================
Comment at: unittests/Transforms/Utils/CodeExtractor.cpp:67
+  EXPECT_TRUE(Outlined);
+  EXPECT_FALSE(verifyFunction(*Outlined));
+}
----------------
Why is `FALSE` expected here?


https://reviews.llvm.org/D37902