[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

I think I've mentioned this before, but I would like to discuss the possibility of adding a target hook(s) for some of these operations (conversions, arithmetic when it comes). Precisely matching the emitted saturation operation is virtually impossible for a target.



================
Comment at: lib/CodeGen/CGExprScalar.cpp:331
+                                  SourceLocation Loc);
+
   /// Emit a conversion from the specified complex type to the specified
----------------
What's the plan for the other conversions (int<->fix, float<->fix)? Functions for those as well?

What about `EmitScalarConversion`? If it cannot handle conversions of fixed-point values it should probably be made to assert, since it will likely mess up.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1017
+    // Need to extend first before scaling up
+    ResultWidth = SrcWidth + DstScale - SrcScale;
+    llvm::Type *UpscaledTy = Builder.getIntNTy(ResultWidth);
----------------
It is definitely simpler to always do the 'upscale+resize/downscale, [saturate], resize' in the constant evaluation case, but when emitting IR it is not as efficient. There's no need to keep the upshifted bits if you aren't interested in saturation, so in the non-saturating case it is better to keep everything in the native type sizes with '[downscale], resize, [upscale]'. This is why there are a bunch of 'sext i32 to i33' in the test cases.

Perhaps something like
```
if !dst.saturating
  downscale if needed
  resize
  upscale if needed
  return 

old code here, minus the 'DstFPSema.isSaturated()' which is implied
```

It gives a bit of duplication (or at least similar code) but I think it's an important disambiguation to make.


================
Comment at: lib/CodeGen/CGExprScalar.cpp:1052
+    } else if (IsSigned && !DstFPSema.isSigned()) {
+      Value *Zero =
+          ConstantInt::get(CGF.getLLVMContext(), APInt(ResultWidth, 0));
----------------
`ConstantInt::getNullValue`?


================
Comment at: lib/Sema/Sema.cpp:537
+  case Type::STK_FixedPoint:
+    llvm_unreachable("Unknown cast from FixedPoint to boolean");
   }
----------------
Will we want to support this?


Repository:
  rC Clang

https://reviews.llvm.org/D50616