Page MenuHomePhabricator

Change semantics of fadd/fmul vector reductions.
ClosedPublic

Authored by sdesmalen on Apr 4 2019, 5:48 AM.

Details

Summary

This patch changes how LLVM handles the accumulator/start value
in the reduction, by never ignoring it regardless of the presence of
fast-math flags on callsites. This change introduces the following
new intrinsics to replace the existing ones:

llvm.experimental.vector.reduce.fadd -> llvm.experimental.vector.reduce.v2.fadd
llvm.experimental.vector.reduce.fmul -> llvm.experimental.vector.reduce.v2.fmul

and adds functionality to auto-upgrade existing LLVM IR and bitcode.

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.Apr 4 2019, 5:48 AM
nikic added a subscriber: nikic.Apr 6 2019, 8:45 AM
simoll added a subscriber: simoll.Apr 7 2019, 9:56 PM
nikic added a comment.May 11 2019, 1:37 AM

Rereading the ML discussion just now, it looks like the consensus is to go with this option. Move forward?

Rereading the ML discussion just now, it looks like the consensus is to go with this option. Move forward?

Thanks for the prod! I've sent an update to the ML.

sdesmalen retitled this revision from [Option A] Change semantics of fadd/fmul vector reductions. to Change semantics of fadd/fmul vector reductions..Jun 7 2019, 1:14 AM
sdesmalen edited the summary of this revision. (Show Details)
spatel added a subscriber: spatel.Jun 7 2019, 5:57 AM
nikic added inline comments.Jun 7 2019, 12:25 PM
test/CodeGen/Generic/expand-experimental-reductions.ll
113 ↗(On Diff #193692)

The expansion above looks wrong per new semantics -- shouldn't there be a use %accum in there? Probably the IR level expansion code needs to be adjusted.

sdesmalen updated this revision to Diff 203685.Jun 8 2019, 6:38 AM
sdesmalen marked an inline comment as done.
  • Fixed ExpandReductions pass to use the accumulator value and updated corresponding tests.
sdesmalen marked an inline comment as done.Jun 8 2019, 6:38 AM
sdesmalen added inline comments.
test/CodeGen/Generic/expand-experimental-reductions.ll
113 ↗(On Diff #193692)

Good spot! I've fixed this now.

nikic accepted this revision.Jun 8 2019, 8:25 AM

LGTM with the FIXME adjusted.

include/llvm/CodeGen/BasicTTIImpl.h
1266 ↗(On Diff #203685)

Regardless of IsPairwiseForm, this will compute an unordered reduction cost, just for two different reduction strategies. Just passing FMF.allowReassoc() here wouldn't be meaningful. We'd need a separate flag to indicate ordered reductions.

This revision is now accepted and ready to land.Jun 8 2019, 8:25 AM
Closed by commit rL363035: Change semantics of fadd/fmul vector reductions. (authored by s.desmalen, committed by ). · Explain WhyJun 11 2019, 1:19 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 11 2019, 1:19 AM
sdesmalen marked 2 inline comments as done.Jun 11 2019, 1:21 AM

Thanks for the review!

include/llvm/CodeGen/BasicTTIImpl.h
1266 ↗(On Diff #203685)

I've updated the FIXME comment to reflect this before committing the patch.

It looks like the documentation change may have broken the sphinx doc job (though I'm not certain it was this change, but the error comes from one of the lines this file touched): http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/32236

sdesmalen marked an inline comment as done.Jun 11 2019, 7:30 AM

It looks like the documentation change may have broken the sphinx doc job (though I'm not certain it was this change, but the error comes from one of the lines this file touched): http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/32236

Thanks for pointing out, for some reason I did not get an automated email about this failure.

I wonder if the 0.0 fp literal is causing the error, although I can't reproduce this locally (maybe due to different version of sphinx?). Probably the simplest fix to get buildbot passing again is to replace the ..code-block:: llvm with ::.

(Note that the LangRef on llvm.org is correctly updated)

It looks like the documentation change may have broken the sphinx doc job (though I'm not certain it was this change, but the error comes from one of the lines this file touched): http://lab.llvm.org:8011/builders/llvm-sphinx-docs/builds/32236

Thanks for pointing out, for some reason I did not get an automated email about this failure.

I wonder if the 0.0 fp literal is causing the error, although I can't reproduce this locally (maybe due to different version of sphinx?). Probably the simplest fix to get buildbot passing again is to replace the ..code-block:: llvm with ::.

(Note that the LangRef on llvm.org is correctly updated)

Sounds good, although I'm not really up to speed with the sphinx system yet, so I can't say for sure that it will work!