This is an archive of the discontinued LLVM Phabricator instance.

[IndVars] Eliminate redundant type cast with different sizes
ClosedPublic

Authored by Allen on Jul 17 2022, 12:13 AM.

Details

Summary

Deal with different sizes between the itofp and fptoi with
trunc or sext/zext, depend on D129756.
Fixes https://github.com/llvm/llvm-project/issues/55505.

Diff Detail

Event Timeline

Allen created this revision.Jul 17 2022, 12:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2022, 12:13 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Allen requested review of this revision.Jul 17 2022, 12:13 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2022, 12:13 AM
nikic added inline comments.Jul 18 2022, 5:41 AM
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
696

Why do we need an early inc range now? We're adding a new user of the IVOperand, not the UseInst, right?

706–721

I don't think the IVOperand is necessarily a phi node. Can you add a test where the itofp is on the postinc IV (i.e. the add)?

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

I don't understand these tests. We don't want an existing sext user, but rather a wider type on the fptosi. So sitofp i16 and then fptosi i32.

Allen updated this revision to Diff 445502.Jul 18 2022, 7:53 AM

Address the comment and add new test sitofp_fptosi_range_zext_postinc with postinc IV

Allen marked 3 inline comments as done and an inline comment as not done.Jul 18 2022, 7:58 AM
Allen added inline comments.
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
696

I thought using make_early_inc_range would increase the robustness of the code referring to 85b4b21c8bb, I'll delete it as it is surely unnecessary.

706–721

I'm kind of curious, besides phi nodes, what other types of inductive variables can be expressed?

Sure, IVOperand does not need to be a phi node at this point.

PS: the new added case sitofp_fptosi_range_zext_postinc with postinc IV

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

Done, thanks for guiding me on how to build the right test cases.

Allen marked 3 inline comments as done.Jul 23 2022, 7:08 AM

ping ?

Allen updated this revision to Diff 450735.Aug 8 2022, 2:05 AM

update to fix llvm/test/Transforms/IndVarSimplify/floating-point-iv.ll

nikic added inline comments.Aug 8 2022, 2:30 AM
llvm/lib/Transforms/Utils/SimplifyIndVar.cpp
713

I believe we only need sext if both fptosi and sitofp are used. If one of them is unsigned, then we can use zext. This would match InstCombine logic.

In any case, we should have a test for the uitofp+fptosi combination.

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

Should be fptoui

180

Same here

Allen updated this revision to Diff 450768.Aug 8 2022, 5:16 AM

If one of them is unsigned, then we can use zext

Allen edited the summary of this revision. (Show Details)Aug 8 2022, 5:35 AM
nikic added inline comments.Aug 8 2022, 7:34 AM
llvm/test/Transforms/IndVarSimplify/floating-point-small-iv.ll
286

This isn't quite what I had in mind. By "postinc IV" I mean that %inc.int gets passed to sitofp, rather than %iv.int. I think your current code may result in a wrong insertion point for this case.

Allen updated this revision to Diff 451035.Aug 8 2022, 8:17 PM
Allen marked 3 inline comments as done.

Fix the crash of the case with 'postinc IV'

nikic added a comment.Aug 9 2022, 2:02 AM

I don't think the last update changed any code?

Allen updated this revision to Diff 451075.Aug 9 2022, 2:48 AM
Allen marked an inline comment as done.
Allen updated this revision to Diff 451076.Aug 9 2022, 2:51 AM

update to fix crash

Allen added a comment.Aug 9 2022, 2:52 AM

I don't think the last update changed any code?

sorry, I upload with an old version, and now it is ok.

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

Done, thanks @nikic very much!

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

Now, I just update code to fix the crash for the case with "postinc IV", and I'll try to improve it next patch as I need some time to get the right insertion point.

nikic added inline comments.Aug 9 2022, 3:05 AM
llvm/test/Transforms/IndVarSimplify/floating-point-small-iv.ll
286

Shouldn't the insertion point simply be CI?

Allen updated this revision to Diff 451099.Aug 9 2022, 4:07 AM

update the insertion point with CI

nikic accepted this revision.Aug 9 2022, 4:12 AM

LGTM

This revision is now accepted and ready to land.Aug 9 2022, 4:12 AM
Allen updated this revision to Diff 451107.Aug 9 2022, 4:35 AM

rebase to update test case in file floating-point-iv.ll

This revision was landed with ongoing or failed builds.Aug 9 2022, 9:00 AM
This revision was automatically updated to reflect the committed changes.