rjmccall added a comment. In D82663#2153176 <https://reviews.llvm.org/D82663#2153176>, @ebevhan wrote:
> 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 don't understand. The problem statement as I understood it is that using unsigned intrinsics to do unsigned-with-padding operations is leading to poor code-gen, so you want to start using signed intrinsics, which you can safely do because unsigned-with-padding types are intended to be exactly signed types with a dynamic range restriction to non-negative values. The result of that operation is still logically an unsigned-with-padding value; there's no need to return back a modified semantic that says that the result is really a signed value because it's *not* really a signed value, you're just computing it a different way. I also don't understand why you think need a modified semantics value in the first place as opposed to just using a more complex condition when deciding which intrinsics to use. 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
