Page MenuHomePhabricator

[Fixed Point] Add codegen for conversion between fixed-point and floating point.
AcceptedPublic

Authored by ebevhan on Aug 26 2020, 8:37 AM.

Details

Summary

The patch adds the required methods to FixedPointBuilder
for converting between fixed-point and floating point,
and uses them from Clang.

This depends on D54749.

Diff Detail

Event Timeline

ebevhan created this revision.Aug 26 2020, 8:37 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 26 2020, 8:37 AM
ebevhan requested review of this revision.Aug 26 2020, 8:37 AM
leonardchan added inline comments.Aug 26 2020, 12:22 PM
llvm/include/llvm/IR/FixedPointBuilder.h
181–182

Was there a reason for preferring multiplying by the reciprocal instead of dividing by a power of 2?

I think this was discussed before, but can't seem to remember/find the conversation.

ebevhan added inline comments.Aug 27 2020, 1:28 AM
llvm/include/llvm/IR/FixedPointBuilder.h
181–182

Multiplication should be more efficient than division with the same effect.

However, as @rjmccall mentioned in D54749, this rescaling might not work if the exponent is not large enough to fit the scaled value. This is unlikely for single precision float and larger formats, but it probably won't work for half precision.

I'm not entirely sure what to do about that. I was originally planning on adding an intrinsic that did pretty much the same thing as the one in that patch, but with a scaling factor. However, I figured that it would not be necessary when I noticed that an integer intrinsic was already in the works.

It feels odd to have intrinsics that do pretty much the same thing as another intrinsic but with an internal multiplication baked in. On top of that, it would require adding 4 new intrinsics (signed/unsigned, nonsaturating/saturating) which seems like a bit much.

Perhaps it may be necessary to emit something else when half precision is involved here...

ebevhan updated this revision to Diff 290245.Sep 7 2020, 4:46 AM

Added promotion mechanism.

ebevhan updated this revision to Diff 290267.Sep 7 2020, 5:46 AM

Rebased.

ebevhan updated this revision to Diff 290502.Sep 8 2020, 9:11 AM

Updated method name.

rjmccall added inline comments.Sep 10 2020, 10:26 PM
llvm/include/llvm/IR/FixedPointBuilder.h
126

I like this method name. Does this have the same problem as I asked about in the other patch about really needing to be about whether the *unscaled* fixed-point type fits in the given type?

131

Could you just extract that code out of llvm::ConstantFP::get and put it on llvm::Type? Might be better as a separate patch.

While you're at it, there's also a TypeToFloatSemantics function in Constants.cpp that's completely redundant with llvm::Type::getFltSemantics.

ebevhan updated this revision to Diff 291203.Sep 11 2020, 6:53 AM

Using Type::getFloatingPointTy now.

ebevhan added inline comments.Sep 11 2020, 6:54 AM
llvm/include/llvm/IR/FixedPointBuilder.h
126

It should be the same case here, yes. Both the integral range and the scale matter when determining this.

131

Done in D87512.

ebevhan updated this revision to Diff 291211.Sep 11 2020, 7:28 AM

Fix typo.

Ping. Any further comments?

leonardchan accepted this revision.Sep 21 2020, 12:09 PM

LGTM unless @rjmccall has further comments.

This revision is now accepted and ready to land.Sep 21 2020, 12:09 PM

I had a question in the other patch about whether you should just have a method on FixedPointSemantics that returns the unscaled semantics (since FixedPointSemantics is totally capable of expressing integer types), which would let fitsInFloatSemantics have more obvious semantics. That would affect this as well. But otherwise I have no concerns.