This patch adds debug info support to the dagcombine rule (zext (truncate x)) -> (zext (truncate x)).
Details
Details
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
the dagcombine rule (zext (truncate x)) -> (zext (truncate x)).
Are my eyes going bad (well, they are, but more than usual) or do both sides of this rule say the same thing?
Comment Actions
Your eyes are fine: The code replaces a (zext (truncate x)) with
Op = DAG.getZeroExtendInReg(Op, SDLoc(N), MinVT.getScalarType()); SDValue ZExtOrTrunc = DAG.getZExtOrTrunc(Op, SDLoc(N), VT);
My Selection-DAG-Fu is not very good but I think the relevant change is the inReg part of the zext.
Comment Actions
I'm scared to actually approve it, but I can see the clear parallel with the AND case just afterwards, so it seems like the right thing to do.
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 7803 ↗ | (On Diff #129361) | I'm surprised we need to do this - can we achieve the same by using SDLoc(N0) instead of SDLoc(N) in both the above? |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 7803 ↗ | (On Diff #129361) | Just to make sure we are talking about the same thing, I changed the code to be if (SrcVT.bitsLT(VT)) {
if (!LegalOperations || (TLI.isOperationLegal(ISD::AND, SrcVT) &&
TLI.isOperationLegal(ISD::ZERO_EXTEND, VT))) {
SDValue Op = N0.getOperand(0);
Op = DAG.getZeroExtendInReg(Op, SDLoc(N0), MinVT.getScalarType());
AddToWorklist(Op.getNode());
SDValue ZExtOrTrunc = DAG.getZExtOrTrunc(Op, SDLoc(N0), VT);
// Transfer the debug info; the new node is equivalent to N0.
//DAG.transferDbgValues(N0, ZExtOrTrunc);
return ZExtOrTrunc;
}
}That does *not* preserve the DBG_VALUE. |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 7803 ↗ | (On Diff #129361) | OK - but its very rare to have to call transferDbgValues directly like this - I just have the feeling that there is a better way...... |
| lib/CodeGen/SelectionDAG/DAGCombiner.cpp | ||
|---|---|---|
| 7803 ↗ | (On Diff #129361) | I would be thrilled if there were a more natural way to do this. |
Comment Actions
I'm going to land this as is for now, but please do let me know if there is a more principled way of achieving the same thing! Thanks for you help.
Comment Actions
The transform here is really trunc+zext->and+zext. DAGCombine naturally transfers the debug into from the old zext to the new zext; what the explicit call to transferDbgValues is doing here is transfering debug info from the old *trunc* to the new zext.
It seems a little dubious to transferDbgValues between two nodes which don't have the same type, but I guess it works out here because the debugger implicitly truncates the result of the zext? How exactly does that work? The code could use a better comment to clarify this.
Comment Actions
That's what the top-level comment says, but in this particular code path I don't actually see and AND being generated. What am I missing?
It seems a little dubious to transferDbgValues between two nodes which don't have the same type, but I guess it works out here because the debugger implicitly truncates the result of the zext? How exactly does that work? The code could use a better comment to clarify this.
Debug info also encodes the type (and thus size) of the source variable being represented, so is immune to zero-extend and truncate operations that deal with the value being in a register that is larger than the variable. That said, we don't explicitly check that the extend/truncate operations don't reach inside the variable here.
Comment Actions
That's what the top-level comment says, but in this particular code path I don't actually see and AND being generated. What am I missing?
I see the AND node in the optimized DAG; is it getting generated somewhere else?