[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:
> 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.
No because now, the instruction would be moved around.

================
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:
> 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. 
What do you suggest as a placeholder ? Many things are uniqued so replaceAllUsesWith tends to fail horribly.


http://reviews.llvm.org/D17057