[PATCH] D75169: [ARM] Supporting lowering of half-precision FP arguments and returns in AArch32's backend

[Sjoerd Meijer via Phabricator via cfe-commits]

SjoerdMeijer added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:524
+                                      CallConv))
+    return;
   EVT ValueVT = Val.getValueType();
----------------
efriedma wrote:
> pratlucas wrote:
> > efriedma wrote:
> > > I'm not sure I understand why the standard getCopyFromParts/getCopyToParts codepath doesn't work. Is the issue that it uses FP_ROUND/FP_EXTEND to promote from f16 to f32?
> > Yes, the issue is the usage of FP_ROUND/FP_EXTEND indeed. Those cause the argument to be converted from f16 into f32 - with a `vcvtb.f16.f32` for instance - instead of simply being placed the value in the LSBs as required by the AAPCS.
> That makes sense.
> 
> It feels a little weird to have a TLI method to do the splitting, as opposed to adding an extra check to the shared codepath, but I guess this way is more flexible if someone else needs a similar change in the future.
> 
> One other thing to consider is that we could make f16 a "legal" type for all ARM subtargets with floating-point registers, regardless of whether the target actually has native f16 arithmetic instructions. We do this on AArch64.  That would reduce the number of different ways to handle f16 values, and I think this change would be unnecessary.
> One other thing to consider is that we could make f16 a "legal" type for all ARM subtargets with floating-point registers, regardless of whether the target actually has native f16 arithmetic instructions. We do this on AArch64.

I am partly guilty here and there when I added _Float16 and v8.2 FP16 support. 
When I did this, life was easy for AArch64, because it doesn't have a soft float ABI support and it has or hasn't got FP16 support, and so everything is straightforward. Life became an awful lot less pleasant for AArch32, because of the hard/soft float and different FP16 support, i.e. there are early FPU versions with limited fp16 support for the storage only type (some conversion), and from v8.2 and up the native fp16 instruction. It's been a few years now so can't remember exactly, which is a bit unhelpful,  but somewhere here for these corner cases I got into trouble by treating f16 as a legal type.

But in case you're interested / this might be useful, I think this is the mail that I wrote to the llvm dev list when I got into trouble here (which I actually need to reread too for details):

http://lists.llvm.org/pipermail/llvm-dev/2018-January/120537.html

and as referred in that mail, earlier revisions of this:

https://reviews.llvm.org/D38315

might have taken exactly that approach. Long story short, I am not saying we shouldn't do it, just pointing out some background info. And since we're all a few years wiser now since that happened, perhaps we should try again ;-)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75169