This is an archive of the discontinued LLVM Phabricator instance.

[SCCP] Don't mark edges feasible when resolving undefs
ClosedPublic

Authored by nikic on Jun 3 2022, 6:33 AM.

Details

Summary

As branch on undef is immediate undefined behavior, there is no need to mark one of the edges as feasible. We can leave all the edges non-feasible. In IPSCCP, we can replace the branch with an unreachable terminator.

Unfortunately, while this does make undef resolution much simpler, we do still need the other part of it (which marks instructions as overdefined). I think what we currently do there is somewhat non-optimal, but having some kind of undef resolution phase is probably not avoidable.

Diff Detail

Event Timeline

nikic created this revision.Jun 3 2022, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 6:33 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
nikic requested review of this revision.Jun 3 2022, 6:33 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 3 2022, 6:33 AM
fhahn accepted this revision.Jun 20 2022, 12:54 AM
fhahn added a reviewer: efriedma.

LGTM, this should be fine now that branch-on-undef/poison is considered UB in most places including ValueTracking. It might be good to wait a few days with committing in case Eli has additional thoughts.

llvm/test/Transforms/FunctionSpecialization/bug52821-use-after-free.ll
37

not sure if it would be better to use i1 argument instead here and llvm/test/Transforms/FunctionSpecialization/bug55000-read-uninitialized-value.ll

llvm/test/Transforms/SCCP/indirectbr.ll
77

needs updating

llvm/test/Transforms/SCCP/return-zapped.ll
3–4

needs updating

This revision is now accepted and ready to land.Jun 20 2022, 12:54 AM
nikic updated this revision to Diff 438293.Jun 20 2022, 1:41 AM
nikic marked 2 inline comments as done.

Update some test comments.

nikic added inline comments.Jun 20 2022, 1:45 AM
llvm/test/Transforms/FunctionSpecialization/bug52821-use-after-free.ll
37

I've picked the i1 false in these tests to retain the same behavior that that resolveUndefs would end up picking, so that the test expectation remains the same.

efriedma added inline comments.Jun 20 2022, 2:41 PM
llvm/lib/Transforms/Utils/SCCPSolver.cpp
1450

I don't think the example given in the comment is accurate. The lattice distinguishes between unknown and undef, so PHI nodes should propagate correctly.

The problem is other operations: for example, visitCastInst doesn't propagate undef values. (This is an optimization: if the operand of a cast transitions from undef to a non-undef constant, we don't want to force the result of the cast to overdefined. We might want to consider revisiting this once we're using poison more consistently.)

nikic added inline comments.Jun 20 2022, 2:55 PM
llvm/lib/Transforms/Utils/SCCPSolver.cpp
1450

Yes, allowing the lattice value transition from undef -> constant is the point. We do make use of that for phi nodes as well, because we merge undef and C into C, not overdefined.

efriedma added inline comments.Jun 20 2022, 3:25 PM
llvm/lib/Transforms/Utils/SCCPSolver.cpp
1450

The undef->constant transition, by itself, doesn't imply we need resolvedUndefsIn, I think. The way the transition is defined, a "constant" just means "a specific value, or an undef we can force to that value". Given the way SCCPInstVisitor::visitPHINode works, that actually just works.

The issues come up when you try to mix in other operations; SCCPInstVisitor visitors for arithmetic/casts/etc. with an undef input produce unknown, which is different from what you'd otherwise expect for ValueLattice solving.

nikic updated this revision to Diff 438593.Jun 21 2022, 1:24 AM

Improve comment on resolvedUndefsIn().

This revision was landed with ongoing or failed builds.Jun 22 2022, 1:28 AM
This revision was automatically updated to reflect the committed changes.
fhahn added a comment.Jun 27 2022, 2:34 AM

It looks like this regressed DCE in some cases when using the default optimization pipeline. The reproducer for opt -sccp -jump-threading is:

https://llvm.godbolt.org/z/TbGYvo7x8

declare void @foo()
declare void @bar()

define void @test(i1 %c) {
entry:
  br i1 %c, label %unreachable, label %next

next:
  br i1 undef, label %exit, label %unreachable

exit:
  call void @foo()
  ret void

unreachable:
  call void @bar()
  unreachable
}
nikic added a comment.Jun 27 2022, 8:21 AM

Ideally, we would remove dead blocks during normal SCCP as well, not just IPSCCP, which should avoid this issue. I'll check whether that has any negative compile-time impact.

Alternatively (or preferably: additionally) we can adjust more passes to make use of branch on undef UB. JumpThreading is one of those (I believe it currently tries to pick one of the successors based on some heuristics).

I've put up D128796 for the CFG simplification in scalar SCCP.