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

[Mehdi AMINI via llvm-commits]

joker.eph 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);
----------------
deadalnix wrote:
> 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.
What I mean is that I don't see how removing the "if" entirely would change anything (inline the then body in the parent) here.

================
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.
----------------
deadalnix wrote:
> 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.
Sure but you could (I am not saying you should) use a placeholder with value tracking instead of generating the instruction at the wrong place the first time you see it. 


http://reviews.llvm.org/D17057