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

[John McCall via Phabricator via cfe-commits]

rjmccall added a comment.

In D82663#2144219 <https://reviews.llvm.org/D82663#2144219>, @ebevhan wrote:

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


Just to say "I told you so", I'm pretty sure I told people this would happen. :)

>> 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 that's the best IR pattern to emit, yes.

> If so, why does dealing with the padding bit detail in LLVM rather than Clang make more sense?

Because frontends should be able to just say "I have a value of a type with these semantics, I need you to do these operations, go do them".  The whole purpose of this interface would be to go down a level of abstraction by picking the best IR to represent those operations.

Maybe we're not in agreement about what this interface looks like — I'm imagining something like

  struct FixedPointEmitter {
    IRBuilder &B;
    FixedPointEmitter(IRBuilder &B) : B(B) {}
  
    Value *convert(Value *src, FixedPointSemantics srcSemantics, FixedPointSemantics destSemantics);
    Value *add(Value *lhs, FixedPointSemantics lhsSemantics, Value *rhs, FixedPointSemantics rhsSemantics)
  };



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

Most things in IRBuilder don't have variant representations beyond what's expressed by the value type.  The fact that we've chosen to do so here necessitates a more complex interface.

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

I don't want to tell you the best IR to use to get good code; I just want frontends to have a reasonably canonical interface to use that matches up well with the information we have.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663