[AMDGPU] Respect existing glue when lowering convergence tokens #90834
Conversation
|
@llvm/pr-subscribers-backend-amdgpu Author: Sameer Sahasrabuddhe (ssahasra) ChangesFull diff: https://github.com/llvm/llvm-project/pull/90834.diff 1 Files Affected:
diff --git a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
index cb4efdc7cf657c..68dffdf8060486 100644
--- a/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
+++ b/llvm/lib/Target/AMDGPU/SIISelLowering.cpp
@@ -3859,20 +3859,19 @@ SDValue SITargetLowering::LowerCall(CallLoweringInfo &CLI,
assert(Mask && "Missing call preserved mask for calling convention");
Ops.push_back(DAG.getRegisterMask(Mask));
- if (InGlue.getNode())
- Ops.push_back(InGlue);
-
- // NOTE: This potentially results in *two* glue operands, and the wrong one
- // might possibly show up where the other was intended. In particular,
- // Emitter::EmitMachineNode() expects only the glued convergence token if it
- // exists. Similarly, the selection of the call expects to match only the
- // InGlue operand if it exists.
if (SDValue Token = CLI.ConvergenceControlToken) {
- Ops.push_back(SDValue(DAG.getMachineNode(TargetOpcode::CONVERGENCECTRL_GLUE,
- DL, MVT::Glue, Token),
- 0));
+ SmallVector<SDValue, 2> GlueOps;
+ GlueOps.push_back(Token);
+ if (InGlue.getNode())
+ GlueOps.push_back(InGlue);
+
+ InGlue = SDValue(DAG.getMachineNode(TargetOpcode::CONVERGENCECTRL_GLUE,
+ DL, MVT::Glue, GlueOps), 0);
}
+ if (InGlue.getNode())
+ Ops.push_back(InGlue);
+
SDVTList NodeTys = DAG.getVTList(MVT::Other, MVT::Glue);
// If we're doing a tall call, use a TC_RETURN here rather than an
|
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
|
I guess no nodes actually require this yet. Are you expecting any? Or is this just defensive programming? |
| 0)); | ||
| SmallVector<SDValue, 2> GlueOps; | ||
| GlueOps.push_back(Token); | ||
| if (InGlue.getNode()) |
| } | ||
|
|
||
| if (InGlue.getNode()) |
What is "this" in this context?? When lowering calls, we do glue stuff onto the actually call instruction. Now we also need to glue the convergence token to the same call. The existing attempt happily created a second glue operand, and this change fixes that to have a single glue operand which could itself have a chain of glued operands. |
ssahasra
force-pushed
the
ssahasra/convergence-glue
branch
from
3a5ee8a
to
897cc6d
Compare
"This" meant "this patch". I was trying to work out what kind of patch this is. Does it fix a bug? Why is there no test case? |
This patch merely cements my improved understanding of how glue is supposed to be set on an operation. Callsites already have a glue operand that holds on to the arg nodes. The lowering of control flow tokens at a callsite was adding a second glue operand, which is unexpected. This patch cleans that up by chaining the token with the existing glue operand. In this narrow window inside the AMDGPU backend, this "bug" would have been triggered if those arg operands were actually accessed as glue. But they are not, so nothing broke when I put a second glue operand. |
No description provided.