This is an archive of the discontinued LLVM Phabricator instance.

[LoopIdiomRecognize] Teach detectShiftUntilZeroIdiom to recognize loops where the counter is decrementing.
ClosedPublic

Authored by craig.topper on Dec 7 2020, 12:27 AM.

Details

Summary

This adds support for loops like

unsigned clz(unsigned x) {
    unsigned w = sizeof (x) * 8;
    while (x) {
        w--;
        x >>= 1;
    }

    return w;
}

and

unsigned clz(unsigned x) {
    unsigned w = sizeof (x) * 8 - 1;
    while (x >>= 1) {
        w--;
    }

    return w;
}

To support these we look for add x, -1 as well as add x, 1 that
we already matched. If the value was -1 we need to subtract from
the initial counter value instead of adding to it.

Fixes PR48404.

Diff Detail

Event Timeline

craig.topper created this revision.Dec 7 2020, 12:27 AM
craig.topper requested review of this revision.Dec 7 2020, 12:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 7 2020, 12:27 AM
lebedev.ri edited the summary of this revision. (Show Details)Dec 7 2020, 12:36 AM

Seems to look ok in principle, but needs some negative tests too.

llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
1484–1489

Preexisting problem, but this is kinda broken.
If we have a second add in the loop,
and it happens to be before the one we are looking for,
we'll fail to detect the idiom.
The detection should not involve linear scan over instructions.

llvm/test/Transforms/LoopIdiom/X86/ctlz.ll
677

Precommit this + use update script?

craig.topper added inline comments.Dec 7 2020, 12:50 AM
llvm/test/Transforms/LoopIdiom/X86/ctlz.ll
677

Should use the script on the whole file?

craig.topper added inline comments.Dec 7 2020, 12:58 AM
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
1484–1489

All the fails are continues. So wouldn’t the second add also have to be a recurrence with 1/-1?

lebedev.ri added inline comments.Dec 7 2020, 2:25 AM
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
1484–1489

I guess, but still, this is broken:
https://godbolt.org/z/fzoo61

I.e. my comment is - given that nowadays -indvars will run right afterwards,
should we really care about trying to do it's job.
Should we not *just* transform the loop into countable,
and let it deal with cleaning the old IV's?

Rebase after pre-committing tests

lebedev.ri added inline comments.Dec 14 2020, 2:25 AM
llvm/lib/Transforms/Scalar/LoopIdiomRecognize.cpp
1756–1767

I think this might be marginally less awkward to write this as:

lebedev.ri accepted this revision.Dec 14 2020, 3:16 AM

LGTM, thank you.
I've checked, and alive2 is also happy with this.
As discussed, once D91038 patchset makes progress, i'd like to either subsume this functionality, or at least rip out indvar canonicalization.

This revision is now accepted and ready to land.Dec 14 2020, 3:16 AM
This revision was landed with ongoing or failed builds.Dec 14 2020, 2:25 PM
This revision was automatically updated to reflect the committed changes.