Page MenuHomePhabricator

[Intrinsic] Unsigned Fixed Point Multiplication Intrinsic
ClosedPublic

Authored by leonardchan on Dec 12 2018, 3:39 PM.

Details

Summary

Add an intrinsic that takes 2 unsigned integers with the scale of them provided as the third argument and performs fixed point multiplication on them.

This is a part of implementing fixed point arithmetic in clang where some of the more complex operations will be implemented as intrinsics.

Diff Detail

Repository
rL LLVM

Event Timeline

leonardchan created this revision.Dec 12 2018, 3:39 PM

Maybe a test with scale == bitwidth? Other than that it looks good to me.

llvm/docs/LangRef.rst
13024

It takes vectors as well, though. Seems like the smul.fix docs only mention integers as well.

13054

This says smul.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5866

Is there a reason this doesn't reuse the code above?

llvm/lib/IR/Verifier.cpp
4583

This can't be true for umul_fix.

bjope added inline comments.Dec 14 2018, 6:52 AM
llvm/docs/LangRef.rst
13041

As a reader of this document (or rather a user of the intrinsic) I'd assume that the fixed point type is given by the width of the source arguments, together with the scale. So how can a source value not be within range?

leonardchan marked 7 inline comments as done.

Maybe a test with scale == bitwidth? Other than that it looks good to me.

Added

llvm/docs/LangRef.rst
13041

This should actually say result value, not the source value.

llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp
5866

Initially this was because when choosing the opcode, I would have another nested switch stmt, but since I'll also have very similar code for the saturating versions of SMULFIX and UMULFIX, having this switch could save some lines.

I imagine this same question could be applied to the add/sub sat intrinsics and have them squished together.

*ping*

Any more comments on this patch?

RKSimon added inline comments.Jan 9 2019, 2:04 PM
llvm/test/CodeGen/X86/umul_fix.ll
33

nounwind

214

nounwind

263

Can't you keep the vector codegen from scalarizing? See many of the other TargetLowering::expand* instructions which check for legal/custom vector operations before unrolling.

leonardchan marked 3 inline comments as done.
leonardchan added inline comments.
llvm/test/CodeGen/X86/umul_fix.ll
263

Could you clarify what you mean by keeping codegen from scalarizing? Vectorization isn't that high of a priority for us, but it would be nice to allow for accepting and returning vectors.

RKSimon added inline comments.Jan 16 2019, 1:54 AM
llvm/test/CodeGen/X86/umul_fix.ll
263

Look at VectorLegalizer::ExpandAddSubSat + TargetLowering::expandAddSubSat - that does something very similar to what I'm suggesting.

leonardchan marked 3 inline comments as done.
leonardchan added inline comments.
llvm/test/CodeGen/X86/umul_fix.ll
263

Ah I see. Updated to expand both signed and unsigned fixed point multiplication to normal multiplication for scale of 0. Also updated the signed fixed point test.

RKSimon added inline comments.Jan 17 2019, 4:08 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1231

Please can you pull the vectorization change out into its own patch - we'll get that dealt with first as it improves SMULFIX code which should be independent of the addition of UMULFIX in this patch.

leonardchan marked 2 inline comments as done.
leonardchan added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeVectorOps.cpp
1231

Done. Made D56987 a child of this patch.

Updated and rebased

RKSimon accepted this revision.Feb 1 2019, 5:41 AM

LGTM

llvm/docs/LangRef.rst
12982

Please commit the llvm.smul.fix changes separately

This revision is now accepted and ready to land.Feb 1 2019, 5:41 AM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2019, 5:41 AM
This revision was automatically updated to reflect the committed changes.