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

[Bevin Hansson via Phabricator via cfe-commits]

ebevhan added a comment.

In D82663#2140507 <https://reviews.llvm.org/D82663#2140507>, @leonardchan wrote:

> In D82663#2130355 <https://reviews.llvm.org/D82663#2130355>, @ebevhan wrote:
>
> > Well, it's not so much as adding the bit, but adding the information that the bit exists. That means either new intrinsics for all of the operations, or adding flags to the existing ones. That's a fair bit of added complexity. Also, <signed operation> + <clamp to zero> would do virtually the exact same thing as the new unsigned-with-padding operations, so the utility of adding all of it is a bit questionable.
>
>
> Could the work involved just be adding the flags, then in `llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp` for unsigned operations, choosing between the signed/unsigned underlying `ISD` when lowering intrinsics to DAG? I think you could just pass the padding bit to `FixedPointIntrinsicToOpcode` and handle it from there. This is just off the top of my head so I could be missing other things.


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. Changing the semantics of those intrinsics would be unfortunate for targets that have started using them for their own instructions. I don't really think it's an option to move the padding semantic info into the IR; the intrinsic interface is fairly lean, and I think keeping it that way is a good idea.

I could change the emission to not be so heavy-handed and only use signed operations for the intrinsics, rather than everything. That makes EmitFixedPointBinOp a bit messier, though.



================
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).
+  //
----------------
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.


================
Comment at: clang/test/Frontend/fixed_point_add.c:290-294
+  // UNSIGNED-NEXT: [[USA_EXT:%[a-z0-9]+]] = zext i16 [[USA]] to i40
+  // UNSIGNED-NEXT: [[I_EXT:%[a-z0-9]+]] = zext i32 [[I]] to i40
+  // UNSIGNED-NEXT: [[I:%[a-z0-9]+]] = shl i40 [[I_EXT]], 7
+  // UNSIGNED-NEXT: [[SUM:%[0-9]+]] = add i40 [[USA_EXT]], [[I]]
+  // UNSIGNED-NEXT: [[RES:%[a-z0-9]+]] = trunc i40 [[SUM]] to i16
----------------
leonardchan wrote:
> If this is a workaround for not being able to convey the padding bit to LLVM intrinsics, I think we should only limit changes to instances we would use intrinsics.
I suppose this makes sense, but the logic will be a bit more convoluted in that case.

It is true that in most cases, the clamp-to-zero resulting from the signed->unsigned conversion at the end isn't even necessary. For addition, multiplication, division and shift, the result of positive operands can never become negative, so there's no point to the clamp.

It just felt more general to do it for all of them instead of littering EmitFixedPointBinOp with special cases. But perhaps it would be better to deal with each case individually instead. Still feels like that would make the implementation less clean.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663