[PATCH] D57836: [Intrinsic] Unsigned Fixed Point Saturation Multiplication Intrinsic

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/docs/LangRef.rst:13321
+      ; The result in the following could be rounded up to 2.5 or down to 2
+      %res = call i4 @llvm.umul.fix.sat.i4(i4 3, i4 3, i32 1)  ; %res = 8 (or 9) (1.5 x 1.5 = 2.25)
+
----------------
Should say: "; %res = 4 (or 5)"


================
Comment at: llvm/docs/LangRef.rst:13323
+
+      ; Saturation
+      %res = call i4 @llvm.umul.fix.sat.i4(i4 7, i4 2, i32 0)  ; %res = 7
----------------
Neither of these examples will overflow as currently written so they do not really describe the effect of saturation (and the results shown are incorrect afaict).

I'm planning to change this into:

```
      ; Saturation
      %res = call i4 @llvm.umul.fix.sat.i4(i4 8, i4 2, i32 0)  ; %res = 15 (8 x 2 -> clamped to 15)
      %res = call i4 @llvm.umul.fix.sat.i4(i4 8, i4 8, i32 2)  ; %res = 15 (2 x 2 -> clamped to 3.75)
```


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2750
+      } else {
+        SatMin = HHZero;
+      }
----------------
I don't get this. Afaict SatMin isn't used for unsigned. 
Anyway, I'm currently thinking about refactoring this part of the code, in a separate patch, before adding the umul.fix.sat support.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/LegalizeIntegerTypes.cpp:2780
+      } else {
+        SatMax = HHZero;
+      }
----------------
Afaict this should be the inverse. We overflow if `HH != 0`, and then we need to clamp. Now we clamp to maximum value when `HH == 0`.


Repository:
  rL LLVM

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

https://reviews.llvm.org/D57836