This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] reassociate FP diff of sums into sum of diffs
ClosedPublic

Authored by spatel on Jun 9 2020, 11:23 AM.

Details

Summary

(a[0] + a[1] + a[2] + a[3]) - (b[0] + b[1] + b[2] +b[3]) -->
(a[0] - b[0]) + (a[1] - b[1]) + (a[2] - b[2]) + (a[3] - b[3])

This should be the last step in solving PR43953:
https://bugs.llvm.org/show_bug.cgi?id=43953

We started emitting reduction intrinsics with D80867/ rGe50059f6b6b3, so it's a relatively easy pattern match now to re-order those ops.
Also, I have not seen any complaints for the switch to intrinsics yet, so I'll propose to remove the "experimental" tag from the intrinsics soon.

Diff Detail

Event Timeline

spatel created this revision.Jun 9 2020, 11:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2020, 11:23 AM
spatel updated this revision to Diff 269616.Jun 9 2020, 11:30 AM

Patch updated:
Corrected the final op - we subtract the 2nd reduction's accumulator variable.

RKSimon accepted this revision.Jun 11 2020, 7:01 AM

LGTM as long as the fast math flags are OK.

I'm not sure if the integer add equivalent combine would be useful (yet)? It probably wouldn't help with PSAD cases tbh.

This revision is now accepted and ready to land.Jun 11 2020, 7:01 AM

LGTM as long as the fast math flags are OK.

That’s the usual logic - if the final value is relaxed, then we assume that calcs leading to that value are equivalently relaxed. Note that to get mixed optimization flags, you likely need compilation with different optimization flags + LTO + inlining to ever get to the state where instructions in 1 function have different FMF.

I'm not sure if the integer add equivalent combine would be useful (yet)? It probably wouldn't help with PSAD cases tbh.

From an IR perspective, we should have that for consistency, but I have not looked at what happens with codegen on that path yet. I'll take a look at that next.

This revision was automatically updated to reflect the committed changes.
nikic added a subscriber: nikic.Jun 14 2020, 6:37 AM

Also, I have not seen any complaints for the switch to intrinsics yet, so I'll propose to remove the "experimental" tag from the intrinsics soon.

In case you missed it, this was recently discussed at https://groups.google.com/d/msg/llvm-dev/ku7Vs38DJmo/i7SiGMuaAQAJ.

Also, I have not seen any complaints for the switch to intrinsics yet, so I'll propose to remove the "experimental" tag from the intrinsics soon.

In case you missed it, this was recently discussed at https://groups.google.com/d/msg/llvm-dev/ku7Vs38DJmo/i7SiGMuaAQAJ.

Thanks for the reminder! I'll post an update on that thread.