This is an archive of the discontinued LLVM Phabricator instance.

Reapply "[SelectionDAG] Selection of DBG_VALUE using a PHI node result (pt 2)"
ClosedPublic

Authored by bjope on May 3 2018, 4:46 AM.

Details

Summary

This reverts SVN r331441 (reapplies r331337), together with a fix
in to handle an already existing fragment expression in the
dbg.value that must be fragmented due to a split PHI node.

This should solve the problem seen in PR37321, which was the
reason for the revert of r331337.

The situation in PR37321 is that we have a PHI node like this

%u.sroa = phi i80 [ %u.sroa.x, %if.x ],
                  [ %u.sroa.y, %if.y ],
                  [ %u.sroa.z, %if.z ]

and a dbg.value like this

call void @llvm.dbg.value(metadata i80 %u.sroa,
                          metadata !13,
                          metadata !DIExpression(DW_OP_LLVM_fragment, 0, 80))

The phi node is split into three 32-bit PHI nodes

%30:gr32 = PHI %11:gr32, %bb.4, %14:gr32, %bb.5, %27:gr32, %bb.8
%31:gr32 = PHI %12:gr32, %bb.4, %15:gr32, %bb.5, %28:gr32, %bb.8
%32:gr32 = PHI %13:gr32, %bb.4, %16:gr32, %bb.5, %29:gr32, %bb.8

but since the original value only is 80 bits we need to adjust the size
of the last fragment expression, and with this patch we get

DBG_VALUE debug-use %30:gr32, debug-use $noreg, !"u", !DIExpression(DW_OP_LLVM_fragment, 0, 32)
DBG_VALUE debug-use %31:gr32, debug-use $noreg, !"u", !DIExpression(DW_OP_LLVM_fragment, 32, 32)
DBG_VALUE debug-use %32:gr32, debug-use $noreg, !"u", !DIExpression(DW_OP_LLVM_fragment, 64, 16)

Diff Detail

Event Timeline

bjope created this revision.May 3 2018, 4:46 AM
bjope updated this revision to Diff 145005.May 3 2018, 5:06 AM

Just a minor change to enhance readability.

I don't know this code so I can't comment on the correctness, but I could at least build the rest of the code that PR37321 came from without issues with this patch.

bjope updated this revision to Diff 145009.May 3 2018, 7:06 AM

Updated to handle more situations (when the original fragment is so small, so we do not even need to use all parts of the split phi node).
Since we now need to be aware about the size of the old fragment at the call to createFragmentExpression I decided to adjust the size already at the caller and not inside createFragmentExpression.

bjope edited the summary of this revision. (Show Details)May 3 2018, 7:06 AM
aprantl accepted this revision.May 3 2018, 8:34 AM
This revision is now accepted and ready to land.May 3 2018, 8:34 AM
This revision was automatically updated to reflect the committed changes.
vsk added inline comments.May 3 2018, 11:29 AM
llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5282 ↗(On Diff #145038)

Just curious -- could this be simplified to 'assert(Offset <= BitsToDescribe)'?

bjope added inline comments.May 3 2018, 11:42 AM
llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5282 ↗(On Diff #145038)

No.

If we for example have a IR level PHI node for i64, and i64 isn't legal we can get two PHI nodes in MIR. Each of them defining a 32-bit vreg. So the loop will do two iterations (I=0 and I=1), with RegisterSize=32.

If the dbg.value only describes a fragment of a variable, then for example we could get BitsToDescibe=24
So for I=0,Offset=0 we will create a DBG_VALUE with a 24-bit fragment.
And then for I=0,Offset=32 we will bail out due to Offset>=BitsToDescribe. And we do not need to create a DBG_VALUE for the second register since we already described all 24-bits that the dbg.value described.

vsk added inline comments.May 3 2018, 11:45 AM
llvm/trunk/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5282 ↗(On Diff #145038)

Got it, thanks for explaining.