[PATCH] D70007: [Intrinsic] Add fixed point division intrinsics.

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7164
+    SDValue Zero = DAG.getConstant(0, dl, VT);
+    SDValue RemZero = DAG.getSetCC(dl, BoolVT, Rem, Zero, ISD::SETEQ);
+    SDValue QuotNeg = DAG.getSetCC(dl, BoolVT, Quot, Zero, ISD::SETLT);
----------------
leonardchan wrote:
> Ah, I think you want this to instead be `ISD::SETNE` and `RemNonZero` 
Oh, you're right. Really surprised I didn't catch this in my testing.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:7168-7170
+    Quot = DAG.getSelect(dl, VT,
+                         DAG.getNode(ISD::AND, dl, BoolVT, RemZero, QuotNeg),
+                         Sub1, Quot);
----------------
leonardchan wrote:
> There's a corner case here where there won't be a subtraction if the true value of a division is between -1 and 0. For `-6 / 7`, the remainder will be non-zero, but quotient will be zero since sdiv rounds towards zero.
> 
> I think you need to instead check if only one of the signs of either operand is negative to check if the quotient is negative:
> 
> ```
>     SDValue LHSNeg = DAG.getSetCC(dl, BoolVT, LHS, Zero, ISD::SETLT);
>     SDValue RHSNeg = DAG.getSetCC(dl, BoolVT, RHS, Zero, ISD::SETLT);
>     SDValue QuotNeg = DAG.getNode(ISD::XOR, dl, BoolVT, LHSNeg, RHSNeg);
> ```
Yep, I see what you mean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D70007