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

[Craig Topper via Phabricator via llvm-commits]

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


================
Comment at: lib/CodeGen/CodeGenPrepare.cpp:660-661
+  for (auto I = pred_begin(BB), E = pred_end(BB); I != E; ++I) {
+    auto CBI = dyn_cast<CallBrInst>((*I)->getTerminator());
+    if (CBI) {
+      if (DestBB == CBI->getFallthrough())
----------------
void wrote:
> Nit: You can combine these two lines.
Done in my local clone. Also it should be "auto *CBI" not "auto CBI". And I would argue that I is terrible variable name for a predecessor so I'll fix that too.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1246
+
+  // FIXME: support whatever these are.
+  if (I.countOperandBundlesOfType(LLVMContext::OB_deopt))
----------------
void wrote:
> "whether these are" what? :-)
No I think the person who wrote this was throwing their hands up and saying they don't what that is.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2554
+  else if (Fn && Fn->isIntrinsic()) {
+    switch (Fn->getIntrinsicID()) {
+    default:
----------------
void wrote:
> Do you need a switch for this? It seems like you could use an if-then instead.
This appears to have been copied from Invoke and had some cases removed.


================
Comment at: lib/IR/Instructions.cpp:260
 
+unsigned CallBase::getNumSubclassExtraOperandsDynamic() const {
+  assert(getOpcode() == Instruction::CallBr && "Unexpected opcode!");
----------------
void wrote:
> Would it make sense to add "invoke" and "call" here? It can simplify the logic where this is called, and it's better to explain the magic numbers that it returns.
Chandler asked me to do it this way so the more common CallInst and InvokeInst can be handled inline.


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

https://reviews.llvm.org/D53765