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

[Jakub Kuderski via Phabricator via llvm-commits]

kuhar added inline comments.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:739
 
   // Reload the outputs passed in by reference
+  Function::arg_iterator OAI = OutputArgBegin;
----------------
nit: s/reference/reference.
Comments should be full sentences and end with `.`.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:764
+
+    // Store to argument right after the definition of output value
+    auto *OutI = dyn_cast<Instruction>(outputs[i]);
----------------
nit: s/value/value.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:776
+    assert(OAI != newFunction->arg_end() &&
+           "Amount of output arguments should match "
+           "the amount of defined values");
----------------
nit: maybe s/amount/number?


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:786
+      // Since there should be only one struct argument aggregating
+      // all the output values, we shouldn't increase OAI, which always
+      // point to the struct argument, in this case
----------------
Maybe s/increase/increment or even s/increase/advance?


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:787
+      // all the output values, we shouldn't increase OAI, which always
+      // point to the struct argument, in this case
+    } else {
----------------
nit: s/point/points
s/case/case.


================
Comment at: unittests/Transforms/Utils/CodeExtractor.cpp:67
+  EXPECT_TRUE(Outlined);
+  EXPECT_FALSE(verifyFunction(*Outlined));
+}
----------------
myhsu wrote:
> kuhar wrote:
> > Why is `FALSE` expected here?
> verifyFunction would return false if no error
Maybe that's obvious for people more familiar with this part of the codebase, but I'd appreciate a comment explaining that here :)


================
Comment at: unittests/Transforms/Utils/CodeExtractor.cpp:58
+  // CodeExtractor requires the first basic block
+  // to dominate all the other ones
+  Candidates.insert(Candidates.begin(), &Func->getEntryBlock());
----------------
nit: s/ones/ones.


https://reviews.llvm.org/D37902