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

Fri Jul 10 08:18:10 PDT 2020

ebevhan added a comment.

In D82663#2142426 <https://reviews.llvm.org/D82663#2142426>, @rjmccall wrote:

> Would it be sensible to use a technical design more like what the matrix folks are doing, where LLVM provides a small interface for emitting operations with various semantics?  FixedPointSemantics would move to that header, and Clang would just call into it.  That way you get a lot more flexibility in how you generate code, and the Clang IRGen logic is still transparently correct.  If you want to add intrinsics or otherwise change the IR patterns used for various operations, you don't have to rewrite a bunch of Clang IRGen logic every time, you just have to update the tests.  It'd then be pretty straightforward to have internal helper functions in that interface for computing things like whether you should use signed or unsigned intrinsics given the desired FixedPointSemantics.

This seems like a reasonable thing to do for other reasons as well. Also moving the actual APFixedPoint class to LLVM would make it easier to reuse the fixedpoint calculation code for constant folding in LLVM, for example.

> My interest here is mainly in (1) keeping IRGen's logic as obviously correct as possible, (2) not hard-coding a bunch of things that really feel like workarounds for backend limitations, and (3) not complicating core abstractions like FixedPointSemantics with unnecessary extra rules for appropriate use, like having to pass an extra "for codegen" flag to get optimal codegen.  If IRGen can just pass down the high-level semantics it wants to some library that will make intelligent decisions about how to emit IR, that seems best.

Just to clarify something here; would the interface in LLVM still emit signed operations for unsigned with padding?  If so, why does dealing with the padding bit detail in LLVM rather than Clang make more sense? The regular IRBuilder is relatively straightforward in its behavior.  I suspect that if anything, LLVM would be equally unwilling to take to take IRBuilder patches that emitted signed intrinsics for certain unsigned operations only due to a detail in Embedded-C's implementation of fixedpoint support.

I could remove the special behavior from FixedPointSemantics and only deal with it in EmitFixedPointBinOp instead. I agree that the FixedPointSemantics interface is muddied by the extra parameter.
Unless I alter the semantics object it might make EmitFixedPointBinOp rather messy, though.

---

Regarding backend limitations, I guess I could propose an alternate solution. If we change FixedPointSemantics to strip the padding bit for both saturating and nonsaturating operations, it may be possible to detect in isel that the corresponding signed operation could be used instead when we promote the type of an unsigned one. For example, if we emit i15 umul.fix scale 15, we could tell in lowering that i16 smul.fix scale 15 is legal and use that instead. Same for all the other intrinsics, including the non-fixedpoint uadd.sat/usub.sat.

The issue with this approach (which is why I didn't really want to do it) is that it's not testable. No upstream target has these intrinsics marked as legal. I doubt anyone would accept a patch with no tests.
It may also be less efficient than just emitting the signed operations in the first place, because we are forced to trunc and zext in IR before and after every operation.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663