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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

Made it through the optimizer code. Really minor changes here, I think this is looking good. Probable the biggest question marks are in the MI representation.

Also, how best to track on-going work that is going to be prioritized in the short term? Mostyl looking for what would be helpful for you all if the FIXME suggested below (or already in the code) are likely to be lost.



================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:4217-4219
+    if (isa<InvokeInst>(CS.getInstruction()) ||
+        isa<CallBrInst>(CS.getInstruction())) {
+      // Can't remove an invoke or callbr because we cannot change the CFG.
----------------
Check for `.isTerminator()` would I think make this a bit more self-explanatory.


================
Comment at: lib/Transforms/InstCombine/InstructionCombining.cpp:923-925
+    // insert a computation after it without breaking the edge. Same for callbr.
+    if (isa<InvokeInst>(InVal) || isa<CallBrInst>(InVal))
+      if (cast<Instruction>(InVal)->getParent() == NonConstBB)
----------------
May also be a tiny bit clearer by using terminator based terminology and APIs.


================
Comment at: lib/Transforms/Scalar/GVN.cpp:2177-2184
   // We don't currently value number ANY inline asm calls.
   if (CallInst *CallI = dyn_cast<CallInst>(CurInst))
     if (CallI->isInlineAsm())
       return false;
 
+  if (CallBrInst *CBI = dyn_cast<CallBrInst>(CurInst))
+    if (CBI->isInlineAsm())
----------------
Can just directly test on the CallBase now.


================
Comment at: lib/Transforms/Utils/LoopSimplify.cpp:245-246
+      // We can't split indirectbr or callbr edges.
+      if (isa<IndirectBrInst>(PN->getIncomingBlock(i)->getTerminator()) ||
+          isa<CallBrInst>(PN->getIncomingBlock(i)->getTerminator()))
         return nullptr;
----------------
Ok, I've seen this pattern too many times. ;]

I'd suggest adding `isIndirectTerminator` or `hasIndirectControlFlow` or some such?


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1269-1270
   if (isa<PHINode>(I1) || !I1->isIdenticalToWhenDefined(I2) ||
-      (isa<InvokeInst>(I1) && !isSafeToHoistInvoke(BB1, BB2, I1, I2)))
+      (isa<InvokeInst>(I1) && !isSafeToHoistInvoke(BB1, BB2, I1, I2)) ||
+      isa<CallBrInst>(I1))
     return false;
----------------
FIXME as we can likely build an analogous safety predicate to the invoke one for CallBr.


================
Comment at: lib/Transforms/Utils/SimplifyCFG.cpp:1451-1456
     if (const auto *C = dyn_cast<CallInst>(I))
       if (C->isInlineAsm())
         return false;
+    if (const auto *CB = dyn_cast<CallBrInst>(I))
+      if (CB->isInlineAsm())
+        return false;
----------------
Fold into a test on `CallBase`.


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

https://reviews.llvm.org/D53765