[PATCH] D53765: [RFC prototype] Implementation of asm-goto support in LLVM

[Craig Topper via Phabricator via llvm-commits]

craig.topper marked 10 inline comments as done.
craig.topper added inline comments.


================
Comment at: include/llvm/Bitcode/LLVMBitCodes.h:477
+  FUNC_CODE_INST_INVOKE = 13,  // INVOKE:     [attr, fnty, op0,op1, ...]
+  FUNC_CODE_INST_CALLBR = 14,  // CALLBR:     [attr, cc, norm, transfs,
+                               //              fnty, fnid, args...]
----------------
Can this use the 14 or does it need a new number? Not sure how holes get created here.


================
Comment at: include/llvm/IR/Instructions.h:3864
+
+  // FIXME: keep an eye on generating and propagating this!
+  unsigned NumTransfers;
----------------
I should probably drop this comment as I'm not sure what the previous author meant.


================
Comment at: lib/Bitcode/Reader/BitcodeReader.cpp:4294
+      cast<CallBrInst>(I)->setCallingConv(
+          static_cast<CallingConv::ID>((0x7ff & CCInfo) >> bitc::CALL_CCONV));
+      cast<CallBrInst>(I)->setAttributes(PAL);
----------------
Should this be "(CCInfo >> bitc::CALL_CCONV) & CallingConv::MaxID" instead?


================
Comment at: lib/CodeGen/AsmPrinter/AsmPrinterInlineAsm.cpp:441
+            if (MI->getOperand(OpNo).isBlockAddress()) {
+              MCSymbol *Sym = AP->GetBlockAddressSymbol(MI->getOperand(OpNo).getBlockAddress());
+              Sym->print(OS, AP->MAI);
----------------
I think this is an 80 column violation


================
Comment at: lib/IR/Instruction.cpp:789
+          isa<CallBrInst>(this)) &&
+         "Can only set weights for call, invoke and callbr instrucitons");
   SmallVector<uint32_t, 1> Weights;
----------------
instructions is misspelled


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:4259
+      if (isa<InvokeInst>(OldCall))
+        cast<InvokeInst>(OldCall)->setCalledFunction(
+            Constant::getNullValue(CalleeF->getType()));
----------------
Can this be merged using CallBase?


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:4282
 
+    if (isa<CallBrInst>(CS.getInstruction())) {
+      // Can't remove a callbr because we cannot change the CFG.
----------------
Merge with above check for invoke


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:4403
+              return false;
+            SmallVector<BasicBlock *, 16> Transfers = CBI->getTransfers();
+            for (unsigned i = 0, e = Transfers.size(); i < e; ++i)
----------------
Should we have an interface that doesn't require copying the transfer list? Just ask for a specific transfer number instead?


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:929
+    // Same for callbr.
+    if (CallBrInst *CBI = dyn_cast<CallBrInst>(InVal))
+      if (CBI->getParent() == NonConstBB)
----------------
Should we merge these by doing isa<InvokeInst> || isa<CallBrInst> then just casting to Instruction to get the parent?


================
Comment at: lib/Transforms/Utils/Local.cpp:1004
+    auto CBI = dyn_cast<CallBrInst>((*I)->getTerminator());
+    if (CBI) {
+      if (Succ == CBI->getFallthrough())
----------------
Merge the dyn_cast into the if


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D53765/new/

https://reviews.llvm.org/D53765