Page MenuHomePhabricator

[IndVars] Directly use integer induction for FPToSI of float induction.
Needs ReviewPublic

Authored by fhahn on Jul 5 2022, 4:50 AM.

Details

Summary

When promoting a float induction to an integer one, we already proven
that all possible vales will be integers that can be converted back to
floats.

This makes fp -> int conversions of the float induction variable
redundant. Instead of unconditionally replacing all uses of the float
induction with a int -> fp conversion of the integer induction, simplify
fp -> int conversion to the integer induction directly.

Doing this in other passes would be a bit more tricky, as we would need
to prove the restricted range of the integer induction again separately.

Fixes #55505.

Diff Detail

Unit TestsFailed

TimeTest
70 msx64 debian > Clangd Unit Tests._/ClangdTests/TUSchedulerTests::PreambleThrottle
Script: -- /var/lib/buildkite-agent/builds/llvm-project/build/tools/clang/tools/extra/clangd/unittests/./ClangdTests --gtest_filter=TUSchedulerTests.PreambleThrottle

Event Timeline

fhahn created this revision.Jul 5 2022, 4:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 4:50 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
fhahn requested review of this revision.Jul 5 2022, 4:50 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 5 2022, 4:50 AM
nikic added inline comments.Jul 5 2022, 5:00 AM
llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
404

What guarantees that this is an fptosi to the correct type (i32 here)?

fhahn updated this revision to Diff 442266.Jul 5 2022, 5:25 AM

Argh, would have been good if this was mentioned in the issue...

Anyways I updated the code to restrict it to fptosi to types matching the integer phi

fhahn marked an inline comment as done.Jul 5 2022, 5:26 AM
fhahn added inline comments.
llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
404

Ah right, fptosi can narrow or widen the result type. Should be fixed.

reames added a comment.EditedJul 5 2022, 7:52 AM

No objection here, but I want to note that recomputing the range should be as simple as asking SCEV for the range of the new IV. We'd basically just need a match for fptosi of sitofp, and then query the range of the input to the sitofp. We should probably be doing that in addition to this patch.

Edit: getSignedRange after calling computeSCEVAtScope that is.

Allen added a subscriber: Allen.Jul 5 2022, 6:40 PM
Allen added a comment.Jul 6 2022, 7:16 PM

No objection here, but I want to note that recomputing the range should be as simple as asking SCEV for the range of the new IV. We'd basically just need a match for fptosi of sitofp, and then query the range of the input to the sitofp. We should probably be doing that in addition to this patch.

Edit: getSignedRange after calling computeSCEVAtScope that is.

hi @reames , out of interest , I tried to recomputing the range in https://reviews.llvm.org/D129191 as an addition patch.

nikic added inline comments.Jul 7 2022, 2:10 AM
llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
406

Add FPToSI to DeadInsts?

fhahn updated this revision to Diff 442957.Jul 7 2022, 9:24 AM
fhahn marked an inline comment as done.

Add fptosi to DeadInsts as suggested, thanks!

No objection here, but I want to note that recomputing the range should be as simple as asking SCEV for the range of the new IV. We'd basically just need a match for fptosi of sitofp, and then query the range of the input to the sitofp. We should probably be doing that in addition to this patch.

Edit: getSignedRange after calling computeSCEVAtScope that is.

hi @reames , out of interest , I tried to recomputing the range in https://reviews.llvm.org/D129191 as an addition patch.

Thanks for sharing the patch!

fhahn marked an inline comment as done.Jul 7 2022, 9:24 AM
llvm/lib/Transforms/Scalar/IndVarSimplify.cpp
406

Done, thanks!

nikic added a comment.Jul 8 2022, 3:06 AM

As D129191 implements a more general solution, is it still worthwhile to also do this?