This is an archive of the discontinued LLVM Phabricator instance.

[IndVars] Eliminate redundant type cast between integer and float
ClosedPublic

Authored by Allen on Jul 6 2022, 5:29 AM.

Details

Summary

Recompute the range: match for fptosi of sitofp, and then query the range of the input to the sitofp
according the comment on D129140.

Fixes https://github.com/llvm/llvm-project/issues/55505.

Diff Detail

Event Timeline

Allen created this revision.Jul 6 2022, 5:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 5:29 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Allen requested review of this revision.Jul 6 2022, 5:29 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 6 2022, 5:29 AM
nikic added inline comments.Jul 7 2022, 2:16 AM
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
681

This is probably not needed, as SIToFP operand must be an integer, which is always SCEVable.

683

Why do we care about hasOneUse here? This seems at odds with your users() loop below.

687

getSignedRange?

698

How can IVOperand not dominate U? IVOperands dominates UseInst, which dominates all its uses.

699

There may be a type mismatch here: Either we need to limit to the same type, or insert sext/trunc as necessary.

Allen updated this revision to Diff 442850.Jul 7 2022, 3:50 AM
Allen updated this revision to Diff 442854.Jul 7 2022, 4:04 AM
Allen marked 2 inline comments as done.

Add type checking

Allen marked 3 inline comments as done.Jul 7 2022, 4:06 AM
Allen added inline comments.
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
681

done, thanks!

683

yes, it can be deleted, thanks @nikic

687

good catch, thanks!

698

thanks, I updated it with replaceAllUsesWith.

699

thanks, I add limit to the same type now.

nikic added a comment.Jul 7 2022, 7:30 AM

This looks reasonable to me, but I think we could use at least two more tests: 1. Type mismatch between the integer IV and the fptosi result. 2. Case where the transform is *not* valid because there are too many significant bits.

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
679

We don't really need the IVOperand argument, given how casts only have a single operand.

694

We should also add CI to DeadInsts, so it gets DCEd.

699

Can just be Changed = true, no need for |=.

llvm/test/Transforms/IndVarSimplify/floating-point-small-iv.ll
3

Drop the -adce here.

37

For this test, it would be better to use an integer IV from the start, rather than going through the float -> int transform. This makes it clear that your code works even if IndVars is not the producer of the integer IV (which is the key difference to D129140).

Allen updated this revision to Diff 443107.Jul 7 2022, 7:04 PM
Allen marked 3 inline comments as done.

Address commant and add two more tests

Allen marked 5 inline comments as done.Jul 7 2022, 7:09 PM
Allen added inline comments.
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
679

Done

694

Thanks @nikic very much, apply your comment.

699

Done

llvm/test/Transforms/IndVarSimplify/floating-point-small-iv.ll
3

Done, it can be dropped after add CI to DeadInsts according your comment, thanks.

37

Done

nikic accepted this revision.Jul 8 2022, 12:16 AM

LGTM

A natural followup here would be to do the same for the unsigned case.

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
687

nit: Unnecessary empty line

929

Directly -> directly

This revision is now accepted and ready to land.Jul 8 2022, 12:16 AM
nikic added inline comments.Jul 8 2022, 12:17 AM
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
931

You might want to call pushIVUsers(IVOperand, L, Simplified, SimpleIVUsers); here, to queue the potentially new direct uses of IVOperand (which no longer go through the cast pair and might be simplified now).

Allen marked 5 inline comments as done.Jul 8 2022, 1:10 AM

LGTM

A natural followup here would be to do the same for the unsigned case.

Ok, I'll try to address this , thanks for reminding

Allen updated this revision to Diff 443166.Jul 8 2022, 1:34 AM

Address the remain comment, and new call pushIVUsers(IVOperand, L, Simplified, SimpleIVUsers)

Allen updated this revision to Diff 443170.Jul 8 2022, 1:57 AM
Allen marked 3 inline comments as done.

rebase to top

This revision was landed with ongoing or failed builds.Jul 8 2022, 2:07 AM
This revision was automatically updated to reflect the committed changes.

LGTM

A natural followup here would be to do the same for the unsigned case.

Ok, I'll try to address this , thanks for reminding. Address the unsigned case here https://reviews.llvm.org/D129358

llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
687

Done

929

Done

931

Done , thanks very much