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

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

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.

I don't think this is necessarily the same as "legalizing" the intrinsic, but this would at least prevent frontends from second-guessing.



================
Comment at: clang/include/clang/Basic/FixedPoint.h:66
   /// given binary operation.
+  /// Is UnsignedPaddingIsSigned is true, unsigned semantics which would
+  /// otherwise have been unsigned will be signed instead. This is for codegen
----------------
Is -> If


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


================
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
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663