[PATCH] D56900: [Fixed Point Arithmetic] Fixed Point and Integer Conversions

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added inline comments.


================
Comment at: clang/lib/AST/ExprConstant.cpp:9825
+    if (Result.isSigned() && !DstSigned) {
+      Overflow = Result < 0 || Result.ugt(DstMax);
+    } else if (Result.isUnsigned() && DstSigned) {
----------------
The `Result < 0` is more clearly expressed as `Result.isNegative()` I think.


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:1499
+          IsNegFractional, NegResult,
+          Builder.CreateAShr(Result, SrcScale - DstScale, "downscale"));
+    } else {
----------------
I think this block can be simplified significantly by simply rounding up before conversion. So, something like:
```
%lt = icmp slt %val, 0
%rounded = add %val, lowBits(Scale)
%sel = select %lt, %rounded, %val
... rescale/resize ...
```



================
Comment at: clang/lib/Sema/SemaChecking.cpp:11096
+        }
+      }
     }
----------------
Seems like a lot of logic in these blocks is duplicated from the code in ExprConstant.


Repository:
  rC Clang

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

https://reviews.llvm.org/D56900