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

[Chandler Carruth via Phabricator via llvm-commits]

chandlerc added a comment.

A few minor comments.

I assume the big outstanding item here is figuring out the MIR representation? Anything else?



================
Comment at: docs/LangRef.rst:6910
+assembly where additional labels can be provided as locations for the inline
+assembly to return to.
+
----------------
Maybe "branch to" or "jump to"? I don't want to imply that it has to be done via call/return.


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1246
+
+  // FIXME: support whatever these are.
+  if (I.countOperandBundlesOfType(LLVMContext::OB_deopt))
----------------
chandlerc wrote:
> craig.topper wrote:
> > 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.
> Maybe have the verifier reject if we can't lower here? Regardless, the comment still seems to need some amount of fixing.
> 
> `deopt` bundles are a reasonable thing, and just not really a reasonable thing (at this stage) to support on callbr.
(still seems to need update)


================
Comment at: lib/CodeGen/GlobalISel/IRTranslator.cpp:1300
+
+  // FIXME: track probabilities.
+  MachineBasicBlock &ReturnMBB = getMBB(*ReturnBB);
----------------
craig.topper wrote:
> chandlerc wrote:
> > This complex to thread through?
> Original author copied that comment out of visitInvokeInst
(comment remains)


================
Comment at: lib/CodeGen/IndirectBrExpandPass.cpp:151-153
+    // FIXME: this part doesn't properly recognize other uses of blockaddress
+    // expressions, for instance, where they are used to pass labels to
+    // asm-goto. This part of the pass needs a rework.
----------------
chandlerc wrote:
> I think we have to fix this before this goes very far. I don't actually know how this is working in practice in our biggest customer (the Linux Kernel) as it should be using things like retpolines and needing this code to work.
> 
> I also think that this entire pass, while a reasonable thing to have, was a bad idea for retpoline lowering. We found retpolines to be faster than the branch trees created by this in most cases w/ many branches. So this needs a non-trivial update. Not sure who has the time to do this at this point though, I'll ask around w/ folks familiar with this bit of code.
> 
> It would be good to at least update the comment to English prose.
FWIW, I think I now understand -- I think the Linux kernel tester we have isn't building w/ retpolines enabled in the compiler, so we just haven't seen this explode. =[


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:2535-2536
+
+  // Retrieve successors. Look through artificial IR level blocks like
+  // catchswitch for successors.
+  MachineBasicBlock *Return = FuncInfo.MBBMap[I.getSuccessor(0)];
----------------
craig.topper wrote:
> chandlerc wrote:
> > Wait, we handle `catchswitch` combined with this? That seems .... ambitious.
> That was copy and pasted from Invoke. I'll remove it
(still needs fixing)


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

https://reviews.llvm.org/D53765