This is an archive of the discontinued LLVM Phabricator instance.

Match types of accumulator and result for llvm.experimental.vector.reduce.fadd/fmul
ClosedPublic

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

Details

Summary

The scalar start/accumulator value of the fadd- and fmul reduction
should match the result type of the reduction, as well as the vector
element-type of the input vector. Although this was not explicitly
specified in the LangRef, it was taken for granted in code implementing
the reductions. The patch also fixes the LangRef by adding this
constraint.

Diff Detail

Repository
rL LLVM

Event Timeline

sdesmalen created this revision.Apr 4 2019, 5:24 AM
nikic added a subscriber: nikic.Apr 6 2019, 8:44 AM
nikic requested changes to this revision.May 11 2019, 1:14 AM

This looks good, but please also update existing tests that specify three types in the intrinstic name. Here's ones I found:

  • test/CodeGen/X86/haddsub.ll
  • test/CodeGen/X86/vector-reduce-fadd-fast.ll
  • test/CodeGen/X86/vector-reduce-fadd.ll
  • test/CodeGen/X86/vector-reduce-fmul-fast.ll
  • test/CodeGen/X86/vector-reduce-fmul.ll

Looks like AArch64/Generic tests already use the (previously incorrect and now correct) two type form.

This revision now requires changes to proceed.May 11 2019, 1:14 AM
sdesmalen updated this revision to Diff 200009.May 17 2019, 3:19 AM
  • Updated more tests.

This looks good, but please also update existing tests that specify three types in the intrinstic name. Here's ones I found:

  • test/CodeGen/X86/haddsub.ll
  • test/CodeGen/X86/vector-reduce-fadd-fast.ll
  • test/CodeGen/X86/vector-reduce-fadd.ll
  • test/CodeGen/X86/vector-reduce-fmul-fast.ll
  • test/CodeGen/X86/vector-reduce-fmul.ll

Good spot! I've updated these tests now.

aemerson accepted this revision.May 17 2019, 9:05 AM

LGTM.

This revision is now accepted and ready to land.May 19 2019, 1:01 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2019, 2:51 AM