This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] Handle conflicts when computing knownbits of PHI nodes in deadcode; PR65022
ClosedPublic

Authored by goldstein.w.n on Aug 27 2023, 2:10 PM.

Details

Summary

Bug was introduced in: https://reviews.llvm.org/D157807

The prior logic assumed that the information from the knownbits
returned from analyzing the icmp and its operands in the context
basicblock would be consistent.

This is not necessarily the case if we are analyzing deadcode.

For example with (icmp sgt (select cond, 0, 1), -1). If we take
knownbits for the select using knownbits from the operator, we will
know the signbit is zero. If we are analyzing a not-taken from based
on the icmp (deadcode), we will also "know" that the select must
be negative (signbit is one). This will result in a conflict in
knownbits.

The fix is to just give up on analying the phi-node if its deadcode. We 1) don't want to waste time continuing to analyze it and 2) will be removing it (and its dependencies) later.

Diff Detail

Event Timeline

goldstein.w.n created this revision.Aug 27 2023, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2023, 2:10 PM
goldstein.w.n requested review of this revision.Aug 27 2023, 2:10 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 27 2023, 2:10 PM
Allen added a subscriber: Allen.Aug 28 2023, 12:16 AM
nikic added inline comments.Aug 28 2023, 3:50 AM
llvm/test/Analysis/ValueTracking/knownbits-phi-deadcode.ll
2

Please reduce this to a single pass invocation.

goldstein.w.n marked an inline comment as done.

Only relies on loop-delete now

goldstein.w.n added inline comments.Aug 28 2023, 10:08 AM
llvm/test/Analysis/ValueTracking/knownbits-phi-deadcode.ll
2

Done, by chance do you know if there is a way to get llvm-reduce to do this for you?

I ended up going through the -O1 passes manually until I could isolate to a single pass (and get necessary transforms from pre-existing passes) but have to feel there is a better way.

Also tried --print-after-all but that often doesn't give complete IR. Just curious if there is some "good" way to do this or is it just figure it out with pass pipeline + ir dumps?

goldstein.w.n added inline comments.Aug 28 2023, 10:10 AM
llvm/test/Analysis/ValueTracking/knownbits-phi-deadcode.ll
2

Actually I guess in retrospect what I would do is get pass thats failing (easy enough). Run pipeline up to that pass and dump IR. Then just run that single pass. Shouldn't be as complicated as I made it.

RKSimon edited the summary of this revision. (Show Details)Aug 28 2023, 10:11 AM
nikic added inline comments.Aug 28 2023, 10:42 AM
llvm/test/Analysis/ValueTracking/knownbits-phi-deadcode.ll
2

You can use -print-on-crash -print-module-scope. There is also utils/reduce_pipeline.py for cases where that's not feasible.

Also, this test still doesn't look very minimal, can you please run it through llvm-reduce?

Re-reduce test

goldstein.w.n marked an inline comment as done.Aug 28 2023, 11:22 AM

ping. Think this is pretty trivial change and fixes active bug so would like to get it in.

nikic accepted this revision.Aug 29 2023, 11:23 AM

LGTM

llvm/test/Analysis/ValueTracking/knownbits-phi-deadcode.ll
58

Is the uselistorder actually needed?

This revision is now accepted and ready to land.Aug 29 2023, 11:23 AM
This revision was landed with ongoing or failed builds.Sep 1 2023, 12:12 AM
This revision was automatically updated to reflect the committed changes.