[PATCH] D71550: [Intrinsic] Add fixed point saturating division intrinsics.

[Leonard Chan via Phabricator via llvm-commits]

leonardchan accepted this revision.
leonardchan added a comment.
This revision is now accepted and ready to land.

In D71550#1880727 <https://reviews.llvm.org/D71550#1880727>, @ebevhan wrote:

> > Ah I see. Could you clarify though on how handling MIN / -EPS invokes undefined behavior?
>
> It's the same as if we had MIN / -1 in integer arithmetic. The result is MAX+1, so we get overflow, which is UB. The same case happens for our lowering of scaled division, since if we have MIN / -EPS (-EPS is -1 in the integer representation), during widening/promotion we may potentially shift up the LHS by the amount widened, giving us MIN / -EPS again, which causes overflow when we do the integer division.
>
> This doesn't matter for non-saturating division since the operation overflows there as well, but for saturating division we need to check the result and saturate, but the div instruction will cause an exception so we never get to that stage.
>
> > Also, if handling this UB requires an extra bit, it seems odd that we would add one a second time. I could be wrong, but isn't the reason for adding the 1st extra bit (i64 -> i65) is just to force the operands to be promoted? If so, then by the time we check `LHSLead + RHSTrail < Scale + (unsigned)(Saturating && Signed))` in `expandFixedPointDiv`, we would've already gotten the extra bit from the initial promotion.
>
> Yes, we add the extra bit to force promotion, but we have to shift up and consume that bit in order to preserve the saturating behavior (SelectionDAGBuilder.cpp:5477) so we don't win any leading sign bits there. We only get those when we do the promotion, but we are one bit short there because of the pre-promotion, so we end up having to widen again. expandFixedPointDiv (during promotion) and earlyExpandDIVFIX do not need to do this extra shifting since they handle saturation directly.
>
> The premature promotion really makes fixed-point division overly complicated.
>
> > I feel like we should only need to promote once when doing fixed point division.
>
> You are right in that the pre-promotion causes us to lose out on a bit. It would maybe be possible to skip the aforementioned one-bit shift, but then we have to tell promotion that we need one extra bit of saturation, and I don't know how to effectively propagate that information.


Thanks for the clarification. Yeah, this is unfortunate. Not being able to do s.63 saturating multiplication kind of leaves a bad taste in my mouth, but for the purposes of implementing the standard, *I don't think* there's an instance where division for the proposed types can result in this (at least on a typical desktop processor). The main issues I can see down the road are frontend related, like if we perhaps will want to add something like `long long _Fract` that has a signed 64 bit width, or other frontends want to have types of varying scale and width. For now though, this still seems to work for our purposes and I also can't think of anything better sooo...LGTM.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D71550/new/

https://reviews.llvm.org/D71550