[PATCH] D55720: [Intrinsic] Signed Fixed Point Saturation Multiplication Intrinsic

[Leonard Chan via Phabricator via llvm-commits]

leonardchan added a comment.
Herald added a subscriber: jdoerfert.

Oh I forgot to submit these inline comments



================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:5428
   if (!Scale) {
-    if (VT.isVector() && !isOperationLegalOrCustom(ISD::MUL, VT))
-      return SDValue();
-    return DAG.getNode(ISD::MUL, dl, VT, LHS, RHS);
+    // [us]mul.fix(a, b, 0) -> mul(a, b)
+    if (!Saturating && isOperationLegalOrCustom(ISD::MUL, VT)) {
----------------
RKSimon wrote:
> I think you need something like this here (please double check my logic):
> ```
> if (VT.isVector() && !isOperationLegalOrCustom(ISD::SMULO, VT) &&
>     !(!Saturating && isOperationLegalOrCustom(ISD::MUL, VT)))
>   return SDValue(); // unroll
> ```
> And that will let you avoid the return SDValue() below by always defaulting to a scalar ISD::MUL/ISD::SMULO that legalization can handle.
I thought we still want to allow vectors to pass to calls to `MUL` and `SMULO`? Wouldn't this scalarize when we disallow vectors even if `MUL` and `SMULO` are legeal?


================
Comment at: llvm/test/CodeGen/X86/smul_fix_sat.ll:2
+; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
+; RUN: llc < %s -mtriple=x86_64-linux | FileCheck %s --check-prefix=X64
+; RUN: llc < %s -mtriple=i686 -mattr=cmov | FileCheck %s --check-prefix=X86
----------------
bjope wrote:
> The expansion is quite complicated (the splitting in four parts and detecting overflow etc).
> Isn't there a risk that X86 is a typical target that will try to find more optimal solutions and maybe also make SMULFIXSAT legal? Then this test case might not really verify the expand code any longer?
> 
> On the other hand, these test cases are just jibberish to me anyway. I can't tell from looking at the checks that DAGTypeLegalizer::ExpandIntRes_MULFIX is doing the right thing. And it would not really help if using another target. Are there perhaps other ways to test DAGTypeLegalizer, such as unit tests?
> 
> One thing that probably can be done quite easily is to a bunch of tests using constant operands. Verifying that DAGCombiner will constant fold to the expected result after having expanded into legal operations (somehow making sure that DAGCombiner do not constant fold the SMULFIXSAT before it has been expanded, I guess someone will add such DAGCombines sooner or later). That way you might be able to get coverage for all paths through DAGTypeLegalizer::ExpandIntRes_MULFIX. Maybe this test should be in a separate test file.
Yeah I can see how these tests are hard to read. I wasn't aware of other ways this could be tested other than making sure the codegen is the same each time. I have my own scripts with different cases to verify the output is correct, but wasn't sure of any existing widely used method of "taking my IR, running it, and verify the results".

Testing with constant operands seems to produce better looking tests for non-saturating multiplication:

```
define i4 @func() {
; X64-LABEL: func:
; X64:       # %bb.0:
; X64-NEXT:    movb $3, %al 
; X64-NEXT:    retq
  %tmp = call i4 @llvm.smul.fix.i4( i4 3, i4 2 , i32 1)
  ret i4 %tmp
} 
```

where we can immediately tell the result is 3, but there's still branching in the saturating case:

```
define i4 @func2() {
; X64-LABEL: func2:
; X64:       # %bb.0:
; X64-NEXT:    xorl %eax, %eax
; X64-NEXT:    testb %al, %al 
; X64-NEXT:    movb $127, %cl 
; X64-NEXT:    jg .LBB1_2
; X64-NEXT:  # %bb.1:
; X64-NEXT:    movb $3, %cl 
; X64-NEXT:  .LBB1_2:
; X64-NEXT:    movb $-1, %al 
; X64-NEXT:    negb %al 
; X64-NEXT:    movb $-128, %al 
; X64-NEXT:    jl .LBB1_4
; X64-NEXT:  # %bb.3:
; X64-NEXT:    movl %ecx, %eax
; X64-NEXT:  .LBB1_4:
; X64-NEXT:    retq
  %tmp = call i4 @llvm.smul.fix.sat.i4( i4 3, i4 2 , i32 1)
  ret i4 %tmp
}
```

so we can't get something as straightforward as with non-saturating.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D55720