This is an archive of the discontinued LLVM Phabricator instance.

[ValueTracking] It is not safe to execute FDIV/FREM speculatively
AbandonedPublic

Authored by Carrot on Feb 13 2023, 2:56 PM.

Details

Reviewers
nikic
Summary

In function isSafeToSpeculativelyExecuteWithOpcode, integer DIV/REM is marked as unsafe to execute speculatively, but floating point FDIV/FREM is not checked. They are also unsafe for the same reason.

There are some optimization regressions with this patch, such as select.ll. It is because function isSafeToSpeculativelyExecuteWithOpcode is called in several different kinds of speculations.

1 The dest block may be executed but the src block is not executed, like the new test case, it is danger for FDIV.
2 The src block is executed by the dest may not be executed, like select.ll, it is safe for FDIV.

In future we may split isSafeToSpeculativelyExecuteWithOpcode for different speculations to get most optimization opportunities.

Diff Detail

Event Timeline

Carrot created this revision.Feb 13 2023, 2:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2023, 2:56 PM
Carrot requested review of this revision.Feb 13 2023, 2:56 PM
craig.topper added inline comments.
llvm/lib/Analysis/ValueTracking.cpp
4837

I don't think this is correct. x/0.0 is infinity or negative infinity. Unless x is 0.0 in which case it's nan. But in any case its not undefined.

craig.topper added inline comments.Feb 13 2023, 3:20 PM
llvm/lib/Analysis/ValueTracking.cpp
4837

I don't think this is correct. x/0.0 is infinity or negative infinity. Unless x is 0.0 in which case it's nan. But in any case its not undefined.

4837

So I don't understand why this patch is needed. There's nothing undefined about fdiv/frem.

Carrot added inline comments.Feb 13 2023, 3:40 PM
llvm/lib/Analysis/ValueTracking.cpp
4837

I copied it from the following UDiv/URem case. It should be changed to

// x / y causes exception if y == 0.0.
craig.topper added inline comments.Feb 13 2023, 3:51 PM
llvm/lib/Analysis/ValueTracking.cpp
4837

Exceptions are assumed to be masked in the default FP environment. If exceptions aren't masked we wouldn't be able to speculate any FP instruction not just fdiv/frem.

nikic requested changes to this revision.Feb 14 2023, 3:51 AM
nikic added a subscriber: nikic.

As pointed out by @craig.topper, the premise of this patch is incorrect. We can't (generally) speculate a constrained fdiv intrinsic, but we already don't do that.

This revision now requires changes to proceed.Feb 14 2023, 3:51 AM
Carrot abandoned this revision.Feb 14 2023, 7:53 PM