[PATCH] D54719: [Intrinsic] Signed Fixed Point Multiplication Intrinsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.

In D54719#1317563 <https://reviews.llvm.org/D54719#1317563>, @efriedma wrote:

> You might need PromoteIntOp_SMULFIX for certain targets, like 64-bit RISCV.


Added, although RISCV doesn't seem to support [US]MUL_LOHI, so I couldn't add a test for that.



================
Comment at: llvm/docs/LangRef.rst:12830
+value is rounded up or down to the closest representable value. The rounding
+direction is unspecified.
+
----------------
bjope wrote:
> ebevhan wrote:
> > efriedma wrote:
> > > "The rounding direction is unspecified" seems scary to me... is there really no standard for rounding these operations?
> > The Embedded-C standard says that the rounding direction is implementation-defined, and is allowed to be indeterminable. 
> > 
> > I also think it's a bit unfortunate that we can't express the rounding direction, since it inhibits optimization, but locking it down to either rounding up or down will make things annoying for any target with operations that round the other direction.
> > 
> > Saying that it's unspecified lets us properly legalize this to something sensible while allowing targets to implement it with legal operations if they have them.
> > 
> > For what it's worth, the legalization will make it round down (toward negative infinity).
> Maybe it should say "target specific" instead? And then the legalization (and any constant folding etc) could use some target transform hook to decide in which direction to do the rounding (I guess it could be set to up/down/any).
> 
> Does "unspecified" mean that ConstantFolding use one strategy for rounding and legalization another strategy? Or should we only allow constant folding when rounding isn't a problem (such as multiply by zero)?
> 
> Things could still be annoying for a target that does care about rounding. Or maybe it isn't allowed for a target to decide what the implementation-defined behavior should be? Or should it be the same for all targets?
> 
> A first implementation could of course go for only supporting "unspecified" (if it is clear what kind of constant folding and other optimizations that are allowed). If a target needs a "target specific" behavior, then I assume the code could be sprinkled with hooks at a later stage.
> 
> Maybe it should say "target specific" instead? And then the legalization (and any constant folding etc) could use some target transform hook to decide in which direction to do the rounding (I guess it could be set to up/down/any).

I would be ok with adding this later. For now I was focusing on laying down the basic code necessary for just using this intrinsic.

> Does "unspecified" mean that ConstantFolding use one strategy for rounding and legalization another strategy? Or should we only allow constant folding when rounding isn't a problem (such as multiply by zero)?

I initially had it as "target specific" but changed it to "unspecified" since the rest of the LangRef doc seems to use this term for describing either implementation-defined, or undefined, behavior.

> Things could still be annoying for a target that does care about rounding. Or maybe it isn't allowed for a target to decide what the implementation-defined behavior should be? Or should it be the same for all targets?
> A first implementation could of course go for only supporting "unspecified" (if it is clear what kind of constant folding and other optimizations that are allowed). If a target needs a "target specific" behavior, then I assume the code could be sprinkled with hooks at a later stage.

The hooks are a good idea. I figure for optimizations, depending on what's supported (up/down/any), InstCombine and others can use the hook to determine rounding direction.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:624
+  SDValue Op1Promoted = GetPromotedInteger(N->getOperand(0));
+  SDValue Op2Promoted = GetPromotedInteger(N->getOperand(1));
+  EVT PromotedType = Op1Promoted.getValueType();
----------------
efriedma wrote:
> Do you need to sign-extend here?
Yup


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:3290
+SDValue DAGTypeLegalizer::ExpandIntOp_ADDCARRY(SDNode *N) {
+  SDValue Lo, Hi;
+  GetExpandedInteger(N->getOperand(2), Lo, Hi);
----------------
efriedma wrote:
> Maybe worth adding a comment that the operand that needs to be expanded must be the third operand, since the other two operands have the same type as the result.
> 
> And actually, I'm not sure how you would hit this in practice; it seems unusual to split a boolean value.
Oh. You're right, this shouldn't have happened in the first place. When expanding to use `ADDCARRY` in `expandMUL_LOHI`, I accidentally created the constant with a VT that was the same as the operands instead of an `MTV::i1`.

Fixed this, and this method is no longer necessary.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D54719