[PATCH] D59981: [SelectionDAGBuilder] Flush PendingExports before creating INLINEASM_BR node for asm goto.

[Chandler Carruth via Phabricator via llvm-commits]

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

LGTM as long as Eli is ok with the testing arriving after the fact.

In D59981#1448044 <https://reviews.llvm.org/D59981#1448044>, @craig.topper wrote:

> I dont' have an application that managed to hit. I just happened to be trying to understand chains and the optimizations made in selectiondagbuilder and realized I didn't know what I was doing when I created INLINEASM_BR.
>
> I'm not sure PendingLoads really need to be flushed. As long as nothing else between here and the branch flushes the pending loads we should be fine. Their chain outputs will just never be connected to anything. And their data output usages shoudl have already been collected by something ahead of the inlineasm_br or real br. If their data isn't used their dead anyway.
>
> The reason invoke has a call to (void)getRoot() in the EHPad part of lowerInvokable is because at the time that was added the code basically did this for invoke
>
> getControlRoot() to prepare to create the eh_label.
>  create eh_label and call DAG.setRoot()
>  getRoot() to create the call.
>
> The problem was that getRoot() on the create call doesn't call DAG.getRoot() unless PendingLoads is empty. If pending loads isn't empty, it expects the root hasn't changed since the loads were created and that the loads themselves used that root in their input and so just creates a token factor of the load chain outputs and sets that as the root. So the call to getRoot to create the call overwrote the setRoot() call made by the eh_label creation causing the eh_label to be disconnected from the graph. Adding the getRoot call in front of the getControlRoot and eh_label creation fixed this by ensuring the PendingLoads was empty when the call was created. The code has been refactored several times since then and now the CallLoweringInfo object is created outside of lowerInvokable and getRoot is called there. So the (void)getRoot() inside lowerInvokable is probably unnecessary now. And the CLI.setChain(getRoot()) that occurs after creating the eh_label can just be CLI.setChain(DAG.getRoot()) since there are no pending loads to flush.


BTW, thanks for the awesome write up. Some of this maybe should go into a FIXME comment around that `getRoot` to document that maybe we should revisit it.



================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:7924
+    // the inlineasm_br node.
+    Chain = getControlRoot();
+  }
----------------
efriedma wrote:
> There's currently a call to getControlRoot() in visitCallBr.  If I'm understanding correctly, the reason we would want to call it here, instead, is to make sure the relevant copytoreg nodes are scheduled before the  INLINEASM_BR, instead of after it?
> 
> I guess the reason this isn't leading to practical problems is that in most cases, a CopyToReg doesn't actually correspond to any instruction?  And in cases where it does, we get lucky and schedule it before the INLINEASM_BR even though though there's no edge that would enforce it?
> 
> If you can't construct a case where the end result of isel is actually different, it might make sense to construct a testcase that checks `-debug-only=isel` or something like that; not usually a fan of that, but it might be the best alternative here.
+1

But I'm personally happy with this going in as-is, and working on a (somewhat iffy) testing strategy as a follow-up.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D59981