Add support for phi nodes in the LLVM C API test

[deadalnix]

Summary

This required to add binding to Instruction::removeFromParent so that instruction can be forward declared and then moved at the right place.

[mehdi_amini]

tools/llvm-c-test/echo.cpp

179 ↗

(On Diff #47396)

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.

208 ↗

(On Diff #47396)

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?

[deadalnix]

tools/llvm-c-test/echo.cpp

179 ↗

(On Diff #47396)

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.

208 ↗

(On Diff #47396)

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.

[mehdi_amini]

tools/llvm-c-test/echo.cpp

179 ↗

(On Diff #47396)

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.

208 ↗

(On Diff #47396)

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.

[deadalnix]

tools/llvm-c-test/echo.cpp

179 ↗

(On Diff #47396)

No because now, the instruction would be moved around.

208 ↗

(On Diff #47396)

What do you suggest as a placeholder ? Many things are uniqued so replaceAllUsesWith tends to fail horribly.

[mehdi_amini]

tools/llvm-c-test/echo.cpp

195 ↗

(On Diff #47396)

Ok here is what I think the semantic is:

    assert((LLVMIsAArgument(Src) || LLVMIsAInstruction(Src)) && "unexpected");

    // Function argument should always be in the map already.
    auto i = VMap.find(Src);
    if (i != VMap.end())
     return i->second;

    // Instruction only are possible at this point
    assert(LLVMIsAInstruction(Src) && "unexpected");

    auto Builder = LLVMCreateBuilderInContext(Ctx);
   auto BB = DeclareBB(LLVMGetInstructionParent(Src));
   LLVMPositionBuilderAtEnd(Builder, BB);
   auto Dst = CloneInstruction(Src, Builder);
   LLVMDisposeBuilder(Builder);
   return Dst;
}

[deadalnix]

Simplify CloneValue ending

[mehdi_amini]

LGTM

llvm/trunk/include/llvm-c/Core.h

  * will be returned.
  */
 LLVMValueRef LLVMGetPreviousInstruction(LLVMValueRef Inst);
 
 /**
  * Remove and delete an instruction.
  *
  * The instruction specified is removed from its containing building
+ * block but is kept alive.
+ *
+ * @see llvm::Instruction::removeFromParent()
+ */
+void LLVMInstructionRemoveFromParent(LLVMValueRef Inst);
+
+/**
+ * Remove and delete an instruction.
+ *
+ * The instruction specified is removed from its containing building
  * block and then deleted.
  *
  * @see llvm::Instruction::eraseFromParent()
  */
 void LLVMInstructionEraseFromParent(LLVMValueRef Inst);
 
 /**
  * Obtain the code opcode for an individual instruction.

llvm/trunk/lib/IR/Core.cpp

 LLVMValueRef LLVMGetPreviousInstruction(LLVMValueRef Inst) {
   Instruction *Instr = unwrap<Instruction>(Inst);
   BasicBlock::iterator I(Instr);
   if (I == Instr->getParent()->begin())
     return nullptr;
   return wrap(&*--I);
 }
 
+void LLVMInstructionRemoveFromParent(LLVMValueRef Inst) {
+  unwrap<Instruction>(Inst)->removeFromParent();
+}
+
 void LLVMInstructionEraseFromParent(LLVMValueRef Inst) {
   unwrap<Instruction>(Inst)->eraseFromParent();
 }
 
 LLVMIntPredicate LLVMGetICmpPredicate(LLVMValueRef Inst) {
   if (ICmpInst *I = dyn_cast<ICmpInst>(unwrap(Inst)))
     return (LLVMIntPredicate)I->getPredicate();
   if (ConstantExpr *CE = dyn_cast<ConstantExpr>(unwrap(Inst)))

llvm/trunk/test/Bindings/llvm-c/echo.ll

next7:
   %9 = icmp slt i32 %a, %b
   br i1 %9, label %next8, label %unreachable
 next8:
   %10 = icmp sle i32 %a, %b
   br i1 %10, label %next9, label %unreachable
 next9:
   ret i32 0
 }
+
+define i32 @loop(i32 %i) {
+  br label %cond
+cond:
+  %c = phi i32 [ %i, %0 ], [ %j, %do ]
+  %p = phi i32 [ %r, %do ], [ 789, %0 ]
+  %1 = icmp eq i32 %c, 0
+  br i1 %1, label %do, label %done
+do:
+  %2 = sub i32 %p, 23
+  %j = sub i32 %i, 1
+  %r = mul i32 %2, 3
+  br label %cond
+done:
+  ret i32 %p
+}

llvm/trunk/tools/llvm-c-test/echo.cpp

if (LLVMIsAConstant(Src)) {
       if (LLVMIsUndef(Src))
         return LLVMGetUndef(CloneType(Src));
 
       // This kind of constant is not supported.
       report_fatal_error("Unsupported contant type");
     }
 
     // Function argument should always be in the map already.
-    if (LLVMIsAArgument(Src)) {
-      auto i = VMap.find(Src);
-      if (i != VMap.end())
-        return i->second;
-    }
 
-    if (LLVMIsAInstruction(Src)) {
-      auto Ctx = LLVMGetModuleContext(M);
-      auto Builder = LLVMCreateBuilderInContext(Ctx);
-      auto BB = DeclareBB(LLVMGetInstructionParent(Src));
-      LLVMPositionBuilderAtEnd(Builder, BB);
-      auto Dst = CloneInstruction(Src, Builder);
-      LLVMDisposeBuilder(Builder);
-      return Dst;
-    }
+    if (!LLVMIsAInstruction(Src))
+      report_fatal_error("Expected an instruction");
+
 
-    fprintf(stderr, "Could not determine the type of %s\n", Name);
-    exit(-1);
-  }
-
   LLVMValueRef CloneInstruction(LLVMValueRef Src, LLVMBuilderRef Builder) {
     const char *Name = LLVMGetValueName(Src);
     if (!LLVMIsAInstruction(Src))
       report_fatal_error("Expected an instruction");
 
     // Check if this is something we already computed.
     {
       auto i = VMap.find(Src);
-      if (i != VMap.end())
-        return i->second;
+      if (i != VMap.end()) {
+        // 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.
+        auto I = i->second;
+        LLVMInstructionRemoveFromParent(I);
+        LLVMInsertIntoBuilderWithName(Builder, I, Name);
+        return I;
+      }
     }
 
     // We tried everything, it must be an instruction
     // that hasn't been generated already.
     LLVMValueRef Dst = nullptr;
 
     LLVMOpcode Op = LLVMGetInstructionOpcode(Src);
     switch(Op) {
switch(Op) {
       }
       case LLVMICmp: {
         LLVMIntPredicate Pred = LLVMGetICmpPredicate(Src);
         LLVMValueRef LHS = CloneValue(LLVMGetOperand(Src, 0));
         LLVMValueRef RHS = CloneValue(LLVMGetOperand(Src, 1));
         Dst = LLVMBuildICmp(Builder, Pred, LHS, RHS, Name);
         break;
       }
+      case LLVMPHI: {
+        // We need to agressively set things here because of loops.
+        VMap[Src] = Dst = LLVMBuildPhi(Builder, CloneType(Src), Name);
+
+        SmallVector<LLVMValueRef, 8> Values;
+        SmallVector<LLVMBasicBlockRef, 8> Blocks;
+
+        unsigned IncomingCount = LLVMCountIncoming(Src);
+        for (unsigned i = 0; i < IncomingCount; ++i) {
+          Blocks.push_back(DeclareBB(LLVMGetIncomingBlock(Src, i)));
+          Values.push_back(CloneValue(LLVMGetIncomingValue(Src, i)));
+        }
+
+        LLVMAddIncoming(Dst, Values.data(), Blocks.data(), IncomingCount);
+        return Dst;
+      }
       case LLVMCall: {
         SmallVector<LLVMValueRef, 8> Args;
         int ArgCount = LLVMGetNumArgOperands(Src);
         for (int i = 0; i < ArgCount; i++)
           Args.push_back(CloneValue(LLVMGetOperand(Src, i)));
         LLVMValueRef Fn = CloneValue(LLVMGetCalledValue(Src));
         Dst = LLVMBuildCall(Builder, Fn, Args.data(), ArgCount, Name);
         break;