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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

> Ah, that's unfortunate... It's a consequence of being forced to handle the integer division overflow case gracefully.
> 
> With an i64, we get a sext+shl and an i65 after building. During the initial promotion we try to promote to i128. That gives us 63 bits of redundant sign (128-65), but in order to handle the case of MSB / ALL1 (MIN / -EPS) without invoking undefined behavior in the integer division, we require an extra bit. Since we don't have that bit to spare, we end up not being able to do the operation in i128. Then we get an i256 sdiv and there's no codegen for that.
> 
> I'm unsure what to do to make this look good. It would be convenient to skip the operation altogether in the overflow case, but we can't generate control flow here. I could check for the overflow case and fudge the inputs/result to prevent the overflow, but that feels sort of wrong. Might be the only viable option, though.

Ah I see. Could you clarify though on how handling MIN / -EPC invokes undefined behavior?

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. I feel like we should only need to promote once when doing fixed point division.



================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:894
+  if (SDValue Res = TLI.expandFixedPointDiv(N->getOpcode(), dl, Op1Promoted,
+                                        Op2Promoted, Scale, DAG)) {
+    if (Saturating)
----------------
nit: align the arguments


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71550