[PATCH] D82663: [CodeGen] Have CodeGen for fixed-point unsigned with padding emit signed operations.

Thu Jul 9 12:11:48 PDT 2020

leonardchan added a comment.

> It wouldn't just be restricted to fixed-point intrinsics, though. It would have to be added to intrinsics like uadd.sat and usub.sat as well, which aren't really tied to fixed-point at all.

Oh wait, sorry. I think I'm starting to understand now. You're saying that if you're using the padding bit in the first place, ISel shouldn't need to perform the underlying shift during integral promotions, but we do it anyway still. Yeah it seems a lot of this could be addressed simply by just using the corresponding signed intrinsics.

I guess I'd be ok with purely making this a clang change for now, but if other frontends see interest in the unsigned padding bit then we could migrate this to LLVM down the line.

================
Comment at: clang/lib/Basic/FixedPoint.cpp:143-158
+  // For codegen purposes, make unsigned with padding semantics signed instead.
+  // This means that we will generate signed operations. The result from these
+  // operations is defined, since ending up with a negative result is undefined
+  // for nonsaturating semantics, and for saturating semantics we will
+  // perform a clamp-to-zero in the last conversion to result semantics (since
+  // we are going from saturating signed to saturating unsigned).
+  //
----------------
ebevhan wrote:
> leonardchan wrote:
> > If this is exclusively for codegen purposes with binary operations, would it be clearer to move this to `EmitFixedPointBinOp`? If `UnsignedPaddingIsSigned` doesn't need to be used for stuff like constant evaluation, it might be clearer not to provide it for everyone.
> FixedPointSemantics is immutable except for saturation, unfortunately. I'd end up having to reconstruct the semantics object from scratch immediately after calling getCommonSemantics.
Fair.

Minor nit: Could you rename the parameter to `UnsignedPaddingIsSignedForCG`? Just want to make it clearer that this should specifically be used for codegen only.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663