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

[craig.topper]

Summary

Since INLINEASM_BR is a terminator we need to flush the pending exports before
emitting it. If we don't do this, a TokenFactor can be inserted between it and
the BR instruction emitted to finish the callbr lowering.

It looks like nodes are glued to the INLINEASM_BR so I had to make sure we emit
the TokenFactor before that.

Not sure if we should also flush PendingLoads for asm goto as well.

I'm also not sure how to directly write a test for this. I'm not sure how to get the TokenFactor to cause something to fail.

[chandlerc]

While I think we should figure out how to test this, we shouldn't wait to land what seems like an important correctness fix on figuring out how to test. So mostly looking for a particular change below.

Maybe you can use synthetic asserts somewhere here to get bugpoint to help you reduce a test case if you have an application that manages to hit it?

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7915–7917 ↗

(On Diff #192782)

While this predicate makes sense for why we would *semantically* need to flush loads, I think we need to flush them for INLINEASM_BR even if it doesn't say this because otherwise we will be at risk of disrupting the terminator layout.

[craig.topper]

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.

[craig.topper]

Ping

[craig.topper]

Ping

[craig.topper]

Ping

[craig.topper]

Ping

[efriedma]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7924 ↗

(On Diff #192782)

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.

8292 ↗

(On Diff #192782)

Does this have any practical effect? Nothing can actually use the root after this point, I think. Not a big deal either way, though.

[efriedma]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8292 ↗

(On Diff #192782)

Err, actually, I guess visitCallBr uses the root? In that case, I'm surprised you haven't seen any issues where the inline asm is getting thrown away by instruction selection.

[craig.topper]

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

8292 ↗

(On Diff #192782)

I think ResultValues.empty() will be true for callbr given the current restrictions on no outputs, but I just wanted to explicitly mention CallBr out of paranoia.

[chandlerc]

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

In 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.

lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

7924 ↗

(On Diff #192782)

+1

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

llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp

for (auto &T : TargetConstraints) {
     // FIXME: Could we compute this on OpInfo rather than T?
 
     // Compute the constraint code and ConstraintType to use.
     TLI.ComputeConstraintToUse(T, SDValue());
 
     ExtraInfo.update(T);
   }
 
+
   // We won't need to flush pending loads if this asm doesn't touch
   // memory and is nonvolatile.
   SDValue Flag, Chain = (HasSideEffect) ? getRoot() : DAG.getRoot();
 
+  bool IsCallBr = isa<CallBrInst>(CS.getInstruction());
+  if (IsCallBr) {
+    // If this is a callbr we need to flush pending exports since inlineasm_br
+    // is a terminator. We need to do this before nodes are glued to
+    // the inlineasm_br node.
+    Chain = getControlRoot();
+  }
+
   // Second pass over the constraints: compute which constraint option to use.
   for (SDISelAsmOperandInfo &OpInfo : ConstraintOperands) {
     // If this is an output operand with a matching input operand, look up the
     // matching input. If their types mismatch, e.g. one is an integer, the
     // other is floating point, or their sizes are different, flag it as an
     // error.
     if (OpInfo.hasMatchingInput()) {
       SDISelAsmOperandInfo &Input = ConstraintOperands[OpInfo.MatchingInput];
case InlineAsm::isClobber:
       break;
     }
   }
 
   // Finish up input operands.  Set the input chain and add the flag last.
   AsmNodeOperands[InlineAsm::Op_InputChain] = Chain;
   if (Flag.getNode()) AsmNodeOperands.push_back(Flag);
 
-  unsigned ISDOpc = isa<CallBrInst>(CS.getInstruction()) ? ISD::INLINEASM_BR
-                                                         : ISD::INLINEASM;
+  unsigned ISDOpc = IsCallBr ? ISD::INLINEASM_BR : ISD::INLINEASM;
   Chain = DAG.getNode(ISDOpc, getCurSDLoc(),
                       DAG.getVTList(MVT::Other, MVT::Glue), AsmNodeOperands);
   Flag = Chain.getValue(1);
 
   // Do additional work to generate outputs.
 
   SmallVector<EVT, 1> ResultVTs;
   SmallVector<SDValue, 1> ResultValues;
if (!ResultValues.empty()) {
     setValue(CS.getInstruction(), V);
   }
 
   // Collect store chains.
   if (!OutChains.empty())
     Chain = DAG.getNode(ISD::TokenFactor, getCurSDLoc(), MVT::Other, OutChains);
 
   // Only Update Root if inline assembly has a memory effect.
-  if (ResultValues.empty() || HasSideEffect || !OutChains.empty())
+  if (ResultValues.empty() || HasSideEffect || !OutChains.empty() || IsCallBr)
     DAG.setRoot(Chain);
 }
 
 void SelectionDAGBuilder::emitInlineAsmError(ImmutableCallSite CS,
                                              const Twine &Message) {
   LLVMContext &Ctx = *DAG.getContext();
   Ctx.emitError(CS.getInstruction(), Message);