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 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
