[PATCH] D50616: [Fixed Point Arithmetic] FixedPointCast

Fri Aug 17 00:50:41 PDT 2018

ebevhan added a comment.

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.

================
Comment at: lib/CodeGen/CGExprScalar.cpp:331
+                                  SourceLocation Loc);
+
   /// Emit a conversion from the specified complex type to the specified
----------------
leonardchan wrote:
> ebevhan wrote:
> > What's the plan for the other conversions (int<->fix, float<->fix)? Functions for those as well?
> > 
> > What about `EmitScalarConversion`? If it cannot handle conversions of fixed-point values it should probably be made to assert, since it will likely mess up.
> Ideally, my plan was to have separate functions for each cast since it seems the logic for each of them is unique, other than saturation which may be abstracted to its own function and be used by the others.
> 
> I wasn't sure if I should also place the logic for these casts in `EmitScalarConversion` since `EmitScalarConversion` seemed pretty large enough and thought it was easier if we instead had separate, self contained functions for each fixed point conversion. But I'm open for hearing ideas on different ways on implementing them.
> 
> Will add the assertion.
Yes, you have a point. Our EmitScalarConversion does all of the fixed-point stuff at once and it's a pretty big mess.

It might be a bit confusing for users if they use EmitScalarConversion, expecting it to work on fixed-point scalars as well but then it doesn't work properly.

So long as it asserts to tell them that, it should be fine but someone else might have an opinion on the expected behavior of the function.

Repository:
  rC Clang

https://reviews.llvm.org/D50616