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

[Min-Yih Hsu via Phabricator via llvm-commits]

myhsu added inline comments.


================
Comment at: lib/Transforms/Utils/CodeExtractor.cpp:785
+      new StoreInst(outputs[i], &*OAI, InsertPt);
+      ++OAI;
+    }
----------------
kuhar wrote:
> 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.
The original code didn't check the OutputArgEnd either, however I add an assertion anyway.

OAI iterator shouldn't be increased in each iteration if AggregateArgs is true. Since under the configuration of using aggregate argument, there would be only one struct argument that accommodate all of the output values in struct fields, and OAI should always point to that struct argument.

The code comment in the original code seems to be not related to the aggregate argument condition, but I add few comment regarding this concern anyway


================
Comment at: unittests/Transforms/Utils/CodeExtractor.cpp:67
+  EXPECT_TRUE(Outlined);
+  EXPECT_FALSE(verifyFunction(*Outlined));
+}
----------------
kuhar wrote:
> Why is `FALSE` expected here?
verifyFunction would return false if no error


https://reviews.llvm.org/D37902