[PATCH] D17057: Add support for phi nodes in the LLVM C API test

[Amaury SECHET via llvm-commits]

deadalnix added inline comments.

================
Comment at: tools/llvm-c-test/echo.cpp:179
@@ -178,3 +178,3 @@
     // Function argument should always be in the map already.
-    if (LLVMIsAArgument(Src)) {
+    if (LLVMIsAArgument(Src) || LLVMIsAInstruction(Src)) {
       auto i = VMap.find(Src);
----------------
joker.eph wrote:
> Why this addition? (the comment talk about function argument)
> 
> The code path seems funky, it seem that the body should always be executed right now or you hit the error.
As to not run CloneInstruction at all if the instruction already was generated.

Argument should be already generated, instruction may or may not be already generated.

================
Comment at: tools/llvm-c-test/echo.cpp:208
@@ +207,3 @@
+        // If we have a hit, it means we already generated the instruction
+        // as a dependancy to somethign else. We need to make sure
+        // it is ordered properly.
----------------
joker.eph wrote:
> s/somethign/something/
> 
> To be sure I understand correctly, in case of a loop you'll generate some instruction (only PHI?) "at the wrong place" and then when you see them again you move them where they should be?
> 
If an instruction use as argument another instruction from a basic block that isn't generated yet, the instruction can't be generated at the right place (this place do not exists yet). So, if we end up there and have a match, it means we are trying to generate an instruction that already where generated as a dependency. We don't need to regenerate the instruction, but we need to make sure it is at the right place.


http://reviews.llvm.org/D17057