This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Add support for more formats in APFixedPoint
ClosedPublic

Authored by Tyker on Sep 27 2022, 3:38 AM.

Details

Summary

Prior to this patch APFixedPoint only support semantics where the LsbWeight is is negative and the Width is at least as large as -LsbWeight.
This patch remove both those requirements.
for example:
with LsbWeight = 2, 12 would be represented as 3.
with LsbWeight = -2, 12 would be represented as 48.

Diff Detail

Event Timeline

Tyker created this revision.Sep 27 2022, 3:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 3:38 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Tyker requested review of this revision.Sep 27 2022, 3:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 27 2022, 3:38 AM

Prior to this patch APFixedPoint only support semantics where the LsbWeight is is negative and the Width is at least as large as -LsbWeight.
This patch remove both those requirements.
for example:
with LsbWeight = 2, 12 would be represented as 3.
with LsbWeight = -2, 12 would be represented as 48.

IIUC should the weights have opposite values in this example? Based off the code, it looks like scale is just negative. So with LsbWeight = -2, then the scale is 2, and 0x1100 (12) is actually 0x11.00 (3). Similarly, LsbWeight = 2, then the scale is -2, and 0x1100 (12) is actually 0x1100|00 (48). Weight doesn't seem to be mentioned in N1169 so I just want to make sure I understand correctly.

llvm/include/llvm/ADT/APFixedPoint.h
43–50

I *think* it should be ok to just have one constructor that takes an int for the LsbWeight.

51

Perhaps place these constants in constexprs before this class, have the bitfields use these constexprs rather than literals, and have a static_assert(sizeof(FixedPointSemantics) == 32) after the class.

Tyker updated this revision to Diff 463481.Sep 28 2022, 3:03 AM
Tyker marked an inline comment as done.

Prior to this patch APFixedPoint only support semantics where the LsbWeight is is negative and the Width is at least as large as -LsbWeight.
This patch remove both those requirements.
for example:
with LsbWeight = 2, 12 would be represented as 3.
with LsbWeight = -2, 12 would be represented as 48.

IIUC should the weights have opposite values in this example? Based off the code, it looks like scale is just negative. So with LsbWeight = -2, then the scale is 2, and 0x1100 (12) is actually 0x11.00 (3). Similarly, LsbWeight = 2, then the scale is -2, and 0x1100 (12) is actually 0x1100|00 (48).

Yes, I used LsbWeight instead of Scale because negative scales looked weird to me and the representation of the semantics in other places use LsbWeight and MsbWeight.

Weight doesn't seem to be mentioned in N1169 so I just want to make sure I understand correctly.

This is not related to N1169. as far as I know fixed points in N1169 don't have negative scales or width smaller then scale.

llvm/include/llvm/ADT/APFixedPoint.h
43–50

C++ would not complain. but it would break the old API, and be hard to use correctly. whereas this change doesn't break the old API. but the syntax to build a FixedPointSemantics with an LsbWeight is heavier.

leonardchan accepted this revision.Sep 28 2022, 10:52 AM

LGTM. Maybe give it a day before committing to see if Bevin has any comments.

llvm/include/llvm/ADT/APFixedPoint.h
43–50

I see. SG

This revision is now accepted and ready to land.Sep 28 2022, 10:52 AM
Tyker closed this revision.Oct 6 2022, 8:56 AM

Thank you for the review.

pushed as 1654b22ac048010a6e045dbe69b530fe7cd54fcc

uabelho added a subscriber: uabelho.EditedOct 7 2022, 4:58 AM

Hi,
Just a heads-up that we see miscompiles with this patch for our out-of-tree target.
It might very well be something in our code that is broken, just thought I'd mention it in case someone else have problems too.
I'll try to debug our problems and see what's up.

Hi,
Just a heads-up that we see miscompiles with this patch for our out-of-tree target.
It might very well be something in our code that is broken, just thought I'd mention it in case someone else have problems too.
I'll try to debug our problems and see what's up.

I don't have any details about how, but it's a cast from a negative fixed point number to an integer that goes wrong now.
So -0.1r is turned into -1 instead of the expected value 0.

Hi,
Just a heads-up that we see miscompiles with this patch for our out-of-tree target.
It might very well be something in our code that is broken, just thought I'd mention it in case someone else have problems too.
I'll try to debug our problems and see what's up.

I don't have any details about how, but it's a cast from a negative fixed point number to an integer that goes wrong now.
So -0.1r is turned into -1 instead of the expected value 0.

I still really don't know how this should work and what's happening but I added some printouts to getIntPart() and got

getIntPart: APFixedPoint(-0.0999755859375, {width=16, scale=15, msb=0, lsb=-15, IsSigned=1, HasUnsignedPadding=0, IsSaturated=0})getMsbWeight(): 0
getLsbWeight(): -15
ExtVal: -3276
Val: -3276
res: -1

So to me it looks like it's converting -0.1 to -1 there, which is wrong i think?

Hi,
Just a heads-up that we see miscompiles with this patch for our out-of-tree target.
It might very well be something in our code that is broken, just thought I'd mention it in case someone else have problems too.
I'll try to debug our problems and see what's up.

I don't have any details about how, but it's a cast from a negative fixed point number to an integer that goes wrong now.
So -0.1r is turned into -1 instead of the expected value 0.

I still really don't know how this should work and what's happening but I added some printouts to getIntPart() and got

getIntPart: APFixedPoint(-0.0999755859375, {width=16, scale=15, msb=0, lsb=-15, IsSigned=1, HasUnsignedPadding=0, IsSaturated=0})getMsbWeight(): 0
getLsbWeight(): -15
ExtVal: -3276
Val: -3276
res: -1

So to me it looks like it's converting -0.1 to -1 there, which is wrong i think?

Yeah the default rounding mode should be towards zero. getIntPart should also return 0 for -0.0999755859375.

llvm/include/llvm/ADT/APFixedPoint.h
216–217

At a glance, I think the issue might be here. The 16-bit wide APSInt is -3276. Prior to this patch, the order was negate Val (3276), shift (3276 >> 15 == 0), then negate again (0). Now the updated process looks to be shift (-3276 >> 15 == -1), negate (1), then negate again (-1). Looks like the right fix is -((-ExtVal).relativeShl(getLsbWeight())). @Tyker could you confirm this?

Tyker added inline comments.Oct 7 2022, 2:13 PM
llvm/include/llvm/ADT/APFixedPoint.h
216–217

You are right, this is incorrect.

uabelho added inline comments.Oct 9 2022, 9:54 PM
llvm/include/llvm/ADT/APFixedPoint.h
216–217

@Tyker : Can you push a fix for this?