[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

[Leonard Chan via Phabricator via cfe-commits]

leonardchan added a comment.

In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote:

> In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote:
>
> > Sorry I forgot to address this also. Just to make sure I understand this correctly since I haven't used these before: target hooks are essentially for emitting target specific code for some operations right? Does this mean that the `EmitFixedPointConversion` function should be moved to a virtual method under `TargetCodeGenInfo` that can be overridden and this is what get's called instead during conversion?
>
>
> Yes, the thought I had was to have a virtual function in TargetCodeGenInfo that would be called first thing in EmitFixedPointConversion, and if it returns non-null it uses that value instead. It's a bit unfortunate in this instance as the only thing that doesn't work for us is the saturation, but letting you configure *parts* of the emission is a bit too specific.


Would it be simpler instead just to have the logic contained in the virtual function for `TargetCodeGenInfo` as opposed to returning `nullptr` since any custom target will end up overriding it anyway and ideally not return `nullptr`?

In https://reviews.llvm.org/D50616#1203751, @rjmccall wrote:

> In https://reviews.llvm.org/D50616#1203692, @ebevhan wrote:
>
> > In https://reviews.llvm.org/D50616#1203446, @leonardchan wrote:
> >
> > > Sorry I forgot to address this also. Just to make sure I understand this correctly since I haven't used these before: target hooks are essentially for emitting target specific code for some operations right? Does this mean that the `EmitFixedPointConversion` function should be moved to a virtual method under `TargetCodeGenInfo` that can be overridden and this is what get's called instead during conversion?
> >
> >
> > Yes, the thought I had was to have a virtual function in TargetCodeGenInfo that would be called first thing in EmitFixedPointConversion, and if it returns non-null it uses that value instead. It's a bit unfortunate in this instance as the only thing that doesn't work for us is the saturation, but letting you configure *parts* of the emission is a bit too specific.
> >
> > In https://reviews.llvm.org/D50616#1203635, @rjmccall wrote:
> >
> > > If this is more than just a hobby — like if there's a backend that wants to do serious work with matching fixed-point operations to hardware support — we should just add real LLVM IR support for fixed-point types instead of adding a bunch of frontend customization hooks.  I don't know what better opportunity we're expecting might come along to justify that, and I don't think it's some incredibly onerous task to add a new leaf to the LLVM type hierarchy.  Honestly, LLVM programmers across the board have become far too accustomed to pushing things opaquely through an uncooperative IR with an obscure muddle of intrinsics.
> > >
> > > In the meantime, we can continue working on this code.  Even if there's eventually real IR support for fixed-point types, this code will still be useful; it'll just become the core of some legalization pass in the generic backend.
> >
> >
> > It definitely is; our downstream target requires it. As far as I know there's no real use case upstream, which is why it's likely quite hard to add any extensions to LLVM proper. Our implementation is done through intrinsics rather than an extension of the type system, as fixed-point numbers are really just integers under the hood. We've always wanted to upstream our fixed-point support (or some reasonable adaptation of it) but never gotten the impression that it would be possible since there's no upstream user.
> >
> > A mail I sent to the mailing list a while back has more details on our implementation, including what intrinsics we've added: http://lists.llvm.org/pipermail/cfe-dev/2018-May/058019.html We also have an LLVM pass that converts these intrinsics into pure IR, but it's likely that it won't work properly with some parts of this upstream design.
> >
> > Leonard's original proposal for this work has always been Clang-centric and never planned for implementing anything on the LLVM side, so we need to make due with what we get, but we would prefer as small a diff from upstream as possible.
>
>
> Has anyone actually asked LLVM whether they would accept fixed-point types into IR?  I'm just a frontend guy, but it seems to me that there are advantages to directly representing these operations in a portable way even if there are no in-tree targets providing special support for them.  And there are certainly in-tree targets that could provide such support if someone was motivated to do it.


I haven't brought this up to llvm-dev, but the main reason for why we decided to only implement this in clang was because we thought it would be a lot harder pushing a new type into upstream llvm, especially since a lot of the features can be supported on the frontend using clang's existing infrastructure. We are open to adding fixed point types to LLVM IR in the future though once all the frontend stuff is laid out and if the community is willing to accept it.


Repository:
  rC Clang

https://reviews.llvm.org/D50616