This is an archive of the discontinued LLVM Phabricator instance.

Rename the VECREDUCE_STRICT_{FADD,FMUL} SDNodes to VECREDUCE_SEQ_{FADD,FMUL}.
ClosedPublic

Authored by aemerson on Oct 3 2020, 10:31 PM.

Details

Summary

The STRICT was causing unnecessary confusion. I think SEQ is a more accurate name for what they actually do, and the other obvious option of "ORDERED" has the issue of already having a meaning in FP contexts.

Diff Detail

Event Timeline

aemerson created this revision.Oct 3 2020, 10:31 PM
aemerson requested review of this revision.Oct 3 2020, 10:31 PM
aemerson edited reviewers, added: spatel; removed: sanjay.nm.Oct 3 2020, 10:50 PM

AArch64 calls these "strictly-ordered reductions". If those two words are off the table then SEQ sounds fine to me providing we explain what that means (which we do).

llvm/include/llvm/CodeGen/ISDOpcodes.h
1109–1124

-> These reductions are not strictly ordered, ...

aemerson updated this revision to Diff 296023.Oct 3 2020, 11:30 PM

Reword comment.

nikic added a comment.Oct 4 2020, 12:43 AM

The STRICT was causing unnecessary confusion. I think SEQ is a more accurate name for what they actually do, and the other obvious option of "ORDERED" has the issue of already having a meaning in FP contexts.

Out of curiosity, what is the existing usage of "ordered" that conflicts with this one?

RKSimon added inline comments.Oct 4 2020, 1:09 AM
llvm/include/llvm/CodeGen/ISDOpcodes.h
1108–1109

To avoid ambiguity - please can you add a codesnippet to the comment showing the order? Maybe something similar to relaxed variants below showing possible expansions (and maybe what the default legalizing expansion is?).

The STRICT was causing unnecessary confusion. I think SEQ is a more accurate name for what they actually do, and the other obvious option of "ORDERED" has the issue of already having a meaning in FP contexts.

Out of curiosity, what is the existing usage of "ordered" that conflicts with this one?

Ordered FP comparisons, e.g. the "oeq" condition code.

llvm/include/llvm/CodeGen/ISDOpcodes.h
1108–1109

Sure, will do.

aemerson updated this revision to Diff 296228.Oct 5 2020, 10:49 AM

Add more comments.

RKSimon added inline comments.Oct 5 2020, 11:30 AM
llvm/include/llvm/CodeGen/ISDOpcodes.h
1108–1109

Shouldn't that be:

RES = VECREDUCE_SEQ_FADD float ACC, <4 x float> SRC_VEC
1109–1124

Check types - FADD or ADD ?

aemerson updated this revision to Diff 296265.Oct 5 2020, 12:46 PM

Oops, brain was still in the fp-typeless GISel world. Fix types to f32/f16.

dmgreen added inline comments.Oct 5 2020, 12:54 PM
llvm/include/llvm/CodeGen/ISDOpcodes.h
1120–1122

We are nitpicking at this point, but should it be using updated variables:

///   PART_RDX = FADD SRC_VEC[0:3], SRC_VEC[4:7]
///   PART_RDX2 = FADD PART_RDX[0:1], PART_RDX[2:3]
///   RES = FADD PART_RDX2[0], PART_RDX2[1]
aemerson updated this revision to Diff 296272.Oct 5 2020, 12:58 PM
aemerson updated this revision to Diff 296598.Oct 6 2020, 10:24 PM

Remove the scalar input overload for fadd/fmul. Now the signature is simple llvm.reduce.fadd.v4f32 rather than llvm.reduce.fadd.f32.v4f32

Also fix some langref formatting, and fold in the change to use "sequential" instead of "ordered" while I'm there.

aemerson updated this revision to Diff 296601.Oct 6 2020, 10:26 PM

Sigh, attached patch to wrong review. Ignore that update, reverting to earlier version.

dmgreen accepted this revision.Oct 7 2020, 12:32 AM

The accidental ping worked :)

LGTM

This revision is now accepted and ready to land.Oct 7 2020, 12:32 AM
RKSimon accepted this revision.Oct 7 2020, 12:53 AM

LGTM

This revision was landed with ongoing or failed builds.Oct 7 2020, 10:53 AM
This revision was automatically updated to reflect the committed changes.