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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc accepted this revision.
chandlerc added a comment.
This revision is now accepted and ready to land.

This is looking really close. I think we should get some basic sanity checks w/ the kernel, and even if there is an obscure bug or two, land this and move to follow-up patches.

So, LGTM pending some basic sanity checks and addressing some nits below.



================
Comment at: include/llvm/Analysis/SparsePropagation.h:333-334
+  if (TI.isExceptionalTerminator() ||
+      isa<IndirectBrInst>(TI) ||
+      isa<CallBrInst>(TI)) {
     Succs.assign(Succs.size(), true);
----------------
Use `TI.isIndirectTerminator()` here?


================
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;
----------------
craig.topper wrote:
> instructions is misspelled
This should probably be `isa<CallBase>` now with a cleaner string, but not critical to fix in this patch.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:9
 //
-// This file implements the visitCall and visitInvoke functions.
+// This file implements the visitCall, visitInvoke and visitCallBr functions.
 //
----------------
nit: I always prefer the Oxford comma to reduce ambiguity.


================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:4346-4350
+      if (Caller->isTerminator())
+        for (User *U : Caller->users())
           if (PHINode *PN = dyn_cast<PHINode>(U))
-            if (PN->getParent() == II->getNormalDest() ||
-                PN->getParent() == II->getUnwindDest())
-              return false;
+            for (int i = 0, e = Caller->getNumSuccessors(); i != e; ++i)
+              if (PN->getParent() == Caller->getSuccessor(i))
----------------
Wow this seems crazily expensive.

I guess it's OK because currently it can only occur for invoke. But the big-O here is quadratic: NumberOfUses * NumberOfSuccessors.

To make matters worse, this change moves to the generic successor APIs which will put a type check in the inner loop. =[

Maybe the simpler approach is to leave the old code for Invoke, and for CallBr simply return false if there are >0 users (which should be never currently).


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

https://reviews.llvm.org/D53765