Page MenuHomePhabricator

[BPI] Adjust the probability for floating point unordered comparison

Authored by Carrot on Jul 25 2019, 3:45 PM.



Since NaN is very rare in normal programs, so the probability for floating point unordered comparison should be extremely small. Current probability is 3/8, it is too large, this patch changes it to a tiny number.

Diff Detail


Event Timeline

Carrot created this revision.Jul 25 2019, 3:45 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 25 2019, 3:45 PM

I've never looked to this piece of code before. For that reason I would like some one more familiar with this part to make a review.

scanon requested changes to this revision.Jul 26 2019, 6:33 AM
scanon added a subscriber: scanon.
scanon added inline comments.
122 ↗(On Diff #211842)

How was this weight selected? Why 2**20 - 1 in particular? If it's just to match IH_TAKEN_WEIGHT, let's use that value instead of repeating it here.

The comment could probable use a little work, too. I broadly agree with the *effect* of this change, but I don't think that unordered is that rare--rather, NaN is often an indication that something has gone wrong, and it makes sense to move that off the hot path.

This revision now requires changes to proceed.Jul 26 2019, 6:33 AM
Carrot updated this revision to Diff 211944.Jul 26 2019, 8:27 AM
Carrot marked an inline comment as done.

Comment change.

122 ↗(On Diff #211842)

It has no semantic relation with IH_TAKEN_WEIGHT, I looked for an appropriate weight to represent the extremely likely probability, and IH_TAKEN_WEIGHT has the similar effect on control flow, so I used the same number.

Do you still think it's better to use IH_TAKEN_WEIGHT here, or what probability is more appropriate here?

What about

if(isnan(x)) ?

LLVM IR has __isnan call.

isnan() is lowered to fcmp uno, in this case the taken probability may be higher, but still much smaller than ordered result, and the usage of isnan() is already very rare, most of uno comparisons are generated from different math library functions.
On the other side I guess most of the taken uno comparisons in a correct program comes from explicitly isnan() call.

scanon added a comment.Aug 8 2019, 9:03 AM

It's not that NaN is rare in normal programs, or that it indicates a bug in the code. It's that testing for NaN is usually an indication that you're testing for an exceptional case, and it makes sense to move those off the hot path (i.e. NaN is actually pretty common, but the likelihood of handling it on the normal-value path through code is small).

456 ↗(On Diff #211944)

I know nothing about Z architecture; is this codegen change actually an improvement? What architectural considerations are at work here?

Carrot updated this revision to Diff 214392.Aug 9 2019, 9:39 AM
Carrot marked an inline comment as done.
Carrot added inline comments.
456 ↗(On Diff #211944)

In this case, the machine code if conversion only considers branch and a target with non trivial probability, the original probability is 3/8, large enough to enable this if conversion. After reducing the probability, if conversion thinks it is too small, not worth the effort to optimize it.

kpn added a subscriber: kpn.Aug 9 2019, 11:57 AM

What will this do to software that does frequently use NaNs and doesn't treat them as exceptional?

static prediction is intended to help common use cases, so for corner cases when NaNs are used frequently, user will need to resort to source annotation (such as builtin_expect).

xbolva00 accepted this revision.Fri, Sep 6, 11:04 AM

I think this change makes sense for plain static mode.

some benchmark numbers?

I think this patch was motivated by the perf of some (micro)benchmarks attached in @Carrot’s previous patch and patch fixes the perf issue.

But I agree, more benchmark results would be useful.

Carrot added a comment.Fri, Sep 6, 2:27 PM

some benchmark numbers?

It brings back 35% regression of Evgeniy's micro benchmark.

SPEC 2006 is running.

Carrot added a comment.Mon, Sep 9, 2:44 PM

spec 2006 int result is 39.2 vs 39.3, so no change.

This revision was not accepted when it landed; it landed in state Needs Review.Tue, Sep 10, 10:25 AM
This revision was automatically updated to reflect the committed changes.