[PATCH] D53738: [Fixed Point Arithmetic] Fixed Point Addition

[Bjorn Pettersson via Phabricator via cfe-commits]

bjope added inline comments.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3208
+  if (LHSWidth < CommonWidth)
+    LHS = LHSSign ? Builder.CreateSExt(LHS, CommonTy)
+                  : Builder.CreateZExt(LHS, CommonTy);
----------------
`LHS = Builder.CreateIntCast(LHS, CommonTy, LHSSign);`

(I think that it even is possible to remove the condition and always do this (the cast will be a no-op if LHSWidth==CommonWidth))


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3211
+  if (RHSWidth < CommonWidth)
+    RHS = RHSSign ? Builder.CreateSExt(RHS, CommonTy)
+                  : Builder.CreateZExt(RHS, CommonTy);
----------------
Same as above, this can be simplified as:
  RHS = Builder.CreateIntCast(RHS, CommonTy, RHSSign);



================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3214
+
+  // Align scales
+  unsigned CommonScale = std::max({LHSScale, RHSScale, ResultScale});
----------------
Arithmetically I think that it would be enough to align the scale to
```
 std::max(std::min(LHSScale, RHSScale), ResultScale)
```
As adding low fractional bits outside the result precision, when we know that those bits are zero in either of the inputs, won't impact any more significant bits in the result.

Maybe your solution is easier and good enough for a first implementation. And maybe `opt` is smart enough to optimize this anyway. Or maybe codegen could be worse due to not using legal types. Or maybe codegen could be improved due to using sadd.sat in more cases?
Lots of "maybes", so I don't think you need to do anything about this right now. It is just an idea from my side.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:3252
+      llvm::Function *intrinsic = CGF.CGM.getIntrinsic(IID, CommonTy);
+      Result = Builder.CreateCall(intrinsic, {LHS, RHS});
+    }
----------------
It should be possible to do it like this (if you add some line wrapping):
```
Builder.CreateBinaryIntrinsic(ResultSign ? llvm::Intrinsic::sadd_sat : 
 llvm::Intrinsic::uadd_sat, LHS, RHS);

```


Repository:
  rC Clang

https://reviews.llvm.org/D53738