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

Wed Jul 15 07:08:24 PDT 2020

ebevhan added a comment.

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

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

Well, transferring the fixed point concept over to LLVM felt like it would happen sooner or later, for the reasons we've discussed here as well as for other reasons. I'm not sure that the discrepancies between the Clang and LLVM semantics were predicted to be the driving factor behind the move, though.

>>> 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)
>   };

I've spent some time going over this and trying to figure out how it would work. I think the interface seems fine on the surface, but I don't see how it directly solves the issues at hand. Regardless of whether this is factored out to LLVM, we still have the issue that we have to massage the semantic **somewhere** in order to get different behavior for certain kinds of semantics during binop codegen.

Since the binop functions take two different semantics, it must perform conversions internally to get the values to match up before the operation. This would probably just be to the common semantic between the two, and it would then return the Value in the common semantic (since we don't know what to convert back to).

In order for the binop functions to have special behavior for padded unsigned, they would need to modify the common semantic internally in order to get the conversion right. This means that the semantic of the returned Value will **not** be what you would normally get from `getCommonSemantic`, so the caller of the function will have no idea what the semantic of the returned value is.

Even if we only treat it as an internal detail of the binop functions and never expose this 'modified' semantic externally, this means we might end up with superfluous operations since (for padded saturating unsigned) we will be forced to trunc the result by one bit to match the real common semantic before we return.

The only solution I can think of is to also return the semantic of the result Value, which feels like it makes the interface pretty bulky.

I can start off by moving APFixedPoint and FixedPointSemantic to ADT, though. Perhaps I should send an RFC.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D82663