This is an archive of the discontinued LLVM Phabricator instance.

Utilize new SDNode flag functionality to expand current support for fadd

Authored by mcberg2017 on Jun 7 2018, 3:02 PM.

Diff Detail


Event Timeline

mcberg2017 created this revision.Jun 7 2018, 3:02 PM
spatel added a comment.Jun 8 2018, 8:42 AM

There's a lot going on here - in particular, the transforms guarded by 'reassoc'. I'm not sure those are correct as-is.

I know this is tedious, but I have to ask again to split this up and add tests, so we can see diffs for each fold using node-level FMF.

697–698 ↗(On Diff #150411)


759 ↗(On Diff #150411)

Change FIXME: the FMF version is good now; the global check is wrong.

(Might want to comment on the broken global checks in multiple places as an NFC preliminary step.)

Removed negation context...

There are several transforms under the last part of this patch, and I'm not sure if we can do them without 'nsz'. This is very similar to D47335 (ping @wristow in case his memory of these cases is better than mine). In that patch, we decided to require 'nsz' for a bunch of reassociations rather than risk breaking some case that we weren't seeing. Should we do that here too at least as a first step? I'm cursing 'nsz' because I just botched that one in D48085.

Here's an example:
(fadd (fadd x, c1), c2) -> (fadd x, (fadd c1, c2))

define float @fadd_consts(float %x) {
  %a1 = fadd float %x, 4.200000e+01
  %a2 = fadd reassoc float %a1, 1.700000e+01
  ret float %a2

We don't fold that in IR (instcombine) currently unless that 2nd fadd has 'nsz' too.

mcberg2017 updated this revision to Diff 151238.EditedJun 13 2018, 1:24 PM

Added a todo comment and added nsz to the constraint for the collection of transformations marked.

mcberg2017 added inline comments.Jun 13 2018, 1:26 PM
10343 ↗(On Diff #151238)

I think I will change "false" to "considered"

spatel added inline comments.Jun 14 2018, 7:16 AM
10345–10346 ↗(On Diff #151238)

This seems broken independently of this patch - we just said it's not safe to create new constants, and every transform under here creates a new constant. But we only check 'AllowNewConst' for some of the folds?

Also, I updated the 1st transform under here to not check hasOneUse() in rL334608, so you'll need to rebase.

Sync'd up to head of tree, adjusted one test (newly added here), some minor refactoring for 'AllowNewConst' and add the cleanup from SelectionDAG since fmul, fsub and fadd are all updated to handle what getNode was doing (that code is removed).

nhaehnle added inline comments.Jun 15 2018, 3:40 AM
10384–10389 ↗(On Diff #151402)

This transformation and the similar ones below can change the NaN-ness of the result.

For example, consider x = +inf, c = -0.5. In that case, the LHS is NaN (because of the intermediate -inf + +inf), while the RHS is +inf. Is reassoc supposed to be a strong enough condition to allow this?

I don't see a way for a non-NaN result to be transformed into a NaN result, so perhaps this is okay? But this needs to be a conscious decision and it should be mentioned in the comment above that talks about rounding steps.

spatel added inline comments.Jun 15 2018, 3:28 PM
10384–10389 ↗(On Diff #151402)

This is circular reasoning, but LLVM does this fold already in IR as long as we have 'nsz reassoc'. This is also allowed by gcc with the equivalent flags:

Our 'reassoc' definition currently says:
"Allow reassociation transformations for floating-point instructions. This may dramatically change results in floating-point."

We could add to that definition if we can make that clearer? Or as you suggest, we can document the potential differences more clearly here by including the INF example.

Updated comment for reassociation case

nhaehnle added inline comments.Jun 16 2018, 1:42 AM
10384–10389 ↗(On Diff #151402)

Those are good points.

It did catch my eye because the documentation of reassoc led me down the path of thinking that sure, there's going to be differences based on different rounding. But this particular difference isn't caused by rounding differences.

Having slept on it and with your points, I think it's okay to do this transform based on reassoc because it doesn't create new NaNs as far as I can tell. If there were similar cases where new NaNs can be created, I think we should tread more carefully. (Of course "mere" rounding differences can create new NaNs, e.g. by turning inf * tiny number into inf * 0, but those should be expected by floating point practicioners, which makes it okay.)

spatel added inline comments.Jun 18 2018, 8:51 AM
10356–10357 ↗(On Diff #151597)

Shouldn't the 'AllowNewConst' check be moved up here rather than repeated with each transform below?

Again, I think that particular change is independent of the FMF change, so it should be reviewed/committed as a separate patch (preferably before we make the FMF change, so we're not increasing the chance of hitting the bug).

Not sure how/if we can make that difference visible in a regression test with in-tree targets. Remove the existing variable in trunk and see if anything breaks?

mcberg2017 added inline comments.Jun 18 2018, 9:05 AM
10356–10357 ↗(On Diff #151597)

The AllowNewConst change is NFC, is I will track it as a simple check in, then update this review afterwards.

mcberg2017 added inline comments.Jun 18 2018, 10:28 AM
10356–10357 ↗(On Diff #151597)

I wound up keeping some of the refactoring so it is not quite NFC, the review should appear shortly. The resultant change here in this review will make the code change quite small.

D48289 is now required to be checked in before this change.

spatel accepted this revision.Jun 18 2018, 3:39 PM

LGTM - see inline for a nit.

10360–10362 ↗(On Diff #151792)

I'd prefer that we not perpetuate UnsafeFPMath any more than necessary, so I'd say something like:
FP reassociation can have drastic effects on results. For example, inf * -2.0 + inf -> NaN, but reassociation allows that to become inf * -1.0 -> -inf.

This revision is now accepted and ready to land.Jun 18 2018, 3:39 PM
mcberg2017 added inline comments.Jun 18 2018, 4:26 PM
10360–10362 ↗(On Diff #151792)

I am going to remove the comment, if we leave a special message about reassocation, someone can infer it is different from Unsafe and therefore new behavior, when it is not.

This revision was automatically updated to reflect the committed changes.