Page MenuHomePhabricator

[InstCombine] visitTrunc - remove dead trunc(lshr (zext A), C) combine
ClosedPublic

Authored by RKSimon on Tue, Sep 29, 8:02 AM.

Details

Summary

I added additional test coverage at rG7a55989dc4305 - but all are handled independently of this combine and http://lab.llvm.org:8080/coverage/coverage-reports/ indicates the code is never used.

Diff Detail

Event Timeline

RKSimon created this revision.Tue, Sep 29, 8:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptTue, Sep 29, 8:02 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
RKSimon requested review of this revision.Tue, Sep 29, 8:02 AM

SGTM, but please add NFCI to patch's subj to better signal that this really shouldn't cause changes.

lebedev.ri accepted this revision.Tue, Sep 29, 8:11 AM
This revision is now accepted and ready to land.Tue, Sep 29, 8:11 AM

Clearly, as per tests, the motivational patterns are handled elsewhere.

Much like with all of instcombine, i believe this heavily depends on the visitation order.
Which i guess means, this (used to) catch some unusual patterns, that didn't go through the usual def-simplification yet.
Probably lack of this fold causes some other followup folds to fire, or in different order, thus causing different final IR.

FWIW, when removing a redundant fold, it would be helpful to point out which other fold already handles it in the review summary.

For the record - the tests from rG7a55989dc430 are all being caught by a mixture of canEvaluateTruncated inside visitTrunc and the "lshr (zext iM X to iN), C --> zext (lshr X, C) to iN" fold in visitLShr - the bin diffs can probably be attributed to visitTrunc no longer catching it and having to wait for the inner visitLShr call.

BTW - All of these folds are still very weak on non-uniform vector support - the number of places where we're using m_ConstantInt or m_APInt instead of a more general m_Constant eval is really annoying.