Page MenuHomePhabricator

[SimplifyCFG] Skip dbg intrinsics when checking for branch-only BBs.
ClosedPublic

Authored by fhahn on Apr 16 2021, 4:15 AM.

Details

Summary

Debug intrinsics are free to hoist and should be skipped when looking
for terminator-only blocks. As a consequence, we have to delegate to the
main hoisting loop to hoist any dbg intrinsics instead of jumping to the
terminator case directly.

This fixes PR49982.

Diff Detail

Event Timeline

fhahn created this revision.Apr 16 2021, 4:15 AM
fhahn requested review of this revision.Apr 16 2021, 4:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 16 2021, 4:15 AM

I've verified that this fixes the problem I saw. Thanks!

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1442

Since I1NonDbg and I2NonDbg should be identical here I guess it doesn't really matter, but anyway: Wouldn't it make the patch look a little bit simpler if we used I1NonDbg here rather than changing from I1 to I2NonDbg?

fhahn updated this revision to Diff 338065.Apr 16 2021, 4:51 AM

Update to use I1NonDbg instead of I2NonDbg, also clang-format.

llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1442

That's indeed better, thanks. Updated!

lebedev.ri added inline comments.Apr 16 2021, 1:52 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1442

I think this isn't quite right. Sure, they are free to host. But now we will simply always drop them?
I think we should still hoist all identical debug info, and skip the rest of the debuginfo.

fhahn added inline comments.Apr 16 2021, 2:55 PM
llvm/lib/Transforms/Utils/SimplifyCFG.cpp
1442

I think we should now handle dbg intrinsics the same as if hoisting was enabled, because we just delegate the hoisting to the existing code. Here we just skip debug intrinsics for the check whether the only non-debug instruction is a branch. The loop should hoist any debug intrinsics if it can, because we do not change I1 and I2.

In hoist_with_debug2 the dbg.value calls are hoisted. They are dropped for hoist_with_debug3_pr49982, but I think not by the hoisting cod but another transform in SimplifyCFG, as they are also dropped without this patch.

lebedev.ri accepted this revision.Apr 16 2021, 3:02 PM

LGTM, thanks.

This revision is now accepted and ready to land.Apr 16 2021, 3:02 PM