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

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added inline comments.


================
Comment at: llvm/docs/LangRef.rst:12629
+less than the source value, or rounded ip to the closest fixed point value
+greater than the source value. This rounding is dependent on the target.
+
----------------
ebevhan wrote:
> This doc doesn't say what happens on overflow. Truncation/wraparound? Or should we consider it undefined?
> 
> If it says that the rounding is dependent on the target, will the rounding mode be target-configurable and exposed through TLI/TTI? We essentially can't touch these intrinsics (constant folding, optimization, even legalization) otherwise.
> 
> Looking at the legalization sequence, it will definitely lower these into a round-down form. If a target has some legal operations which round up and some non-legal operations, then the legal ones will round up and the non-legal ones will round down. That's sort of messy.
> 
> It might be safer to say that the rounding is indeterminate, but that's even worse for optimization.
Overflow I was going to leave as undefined behavior. Added this into the docs.

By target dependent, I meant that whatever legal operation a target could replace this intrinsic with has the choice of rounding up or down. I'm not sure how bad this would be for optimization, but I imagine rounding isn't something that needs to be touched immediately since with this intrinsic, I'm just attempting to match what the standard says. I think one of the conclusions we also came up to in the long thread on llvm-dev was that, for now, we don't need to do more than what's necessary to implement the spec.

I changed the docs though to say rounding is indeterminable, as the spec says.


================
Comment at: llvm/include/llvm/IR/Intrinsics.td:742
 
 //===------------------------- Fixed Point Intrinsics ---------------------===//
 //
----------------
bjope wrote:
> nit: In the langref you are using "Saturation Arithmetic Intrinsics" and "Fixed Point Arithemetic Instrinsics" as two separate chapters. And here we put all of them into one category "Fixed Point Intrinsics". I'm not sure if the saturating arithmetics should be in a fixed point category.
> 
> Anyway, when adding a int_smul_fix_sat later I guess it will be in the "Fixed Point Arithemetic Instrinsics" in the langref (even if it also is saturating). So maybe it is better to not having this split into two categories after all.
Split into 2 categories. Was also planning on putting the saturating fixed point ones under "Fixed Point Intrinsics".


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2543
+          DAG.getNode(ISD::MUL, dl, VT, LHS, RHS).getNode(), Lo, Hi, NVT, DAG,
+          TargetLowering::MulExpansionKind::OnlyLegalOrCustom, LL, LH, RL, RH))
+    return;
----------------
ebevhan wrote:
> Maybe I'm missing something here, but isn't this just a normal, expanded multiplication?
This should actually be an expansion of SMUL_LOHI since we need the high bits when scaling down the result after multiplication. Updated the code, but one thing I found was that `expandMUL_LOHI` depends on `ADDC` but does not have a check for it. When rerunning the tests, it seems that X86 does not support 32 bit `ADDE` (and `ADDC`).

```
LLVM ERROR: Cannot select: t81: i32,glue = adde t121, t44:1, t80:1
```

so I removed these from the lit tests, but if we want to support expansion of this, I'm not sure if the solution is to add a check into `expandMUL_LOHI` to check `isOperationLegalOrCustom(ISD::ADDC/E, VT)` and if it is not legal, write out the expanded version. I found a comment in `ExpandIntRes_ADDSUB()` saying that there currently does not appear to be a way of generating a value of `MTV::Glue` if we were to expand the result.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2544
+          TargetLowering::MulExpansionKind::OnlyLegalOrCustom, LL, LH, RL, RH))
+    return;
+
----------------
ebevhan wrote:
> If we hit this return, doesn't this mean that legalization failed? Do we want to catch this here?
Added a fatal error report here.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5101
+  if (Scale) {
+    SDValue Hi = DAG.getNode(ISD::MULHS, dl, VT, LHS, RHS);
+    EVT ShiftTy = getShiftAmountTy(VT, DAG.getDataLayout());
----------------
craig.topper wrote:
> ebevhan wrote:
> > Could use a couple comments explaining what we're doing with the values/SRL/SHL.
> > 
> > Does this work if MULHS in VT is of dubious legality?
> Ideally we'd use MUL_LOHI if the target supports it. That should allow X86 to use a single multiply instruction in the test cases.
Added checks to see if we can use `MULHS` or `SMUL_LOHI`.


Repository:
  rL LLVM

https://reviews.llvm.org/D54719