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

[Lucas Prates via Phabricator via cfe-commits]

pratlucas marked an inline comment as done.
pratlucas added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:524
+                                      CallConv))
+    return;
   EVT ValueVT = Val.getValueType();
----------------
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.


================
Comment at: llvm/lib/Target/ARM/ARMCallingConv.cpp:296
+
+static bool CC_ARM_AAPCS_Custom_f16(unsigned ValNo, MVT ValVT, MVT LocVT,
+                                    CCValAssign::LocInfo LocInfo,
----------------
efriedma wrote:
> It isn't obvious to me why you need this; can you not use CCBitConvertToType/CCAssignToReg?
For hard floats, using CCAssingToReg would indeed work well in the majority of the scenarios, but would get in the way of the CMSE handling from D81428. Using the f16 loc type causes the clearing of the top 16 bits to be optimized out in the DAG.
Also, the AAPCS expects the argument sized to be extended to 4 bytes, so using the f32 loc type attends to that rule.

For soft floats, on the other hand, simply convering it to i32 causes the code on ARMISel lowering to be quite cumbersome. The loc info becomes either `CCValAssign::BCvt` (f16 -> f32 - >i32) or `CCValAssign::AExt` ( f16 -> i16 -> i32), so checking for when we need to handle things differently for f16 becomes less clear.
Using this flow we have the `isCustom` flag assigned and can have a more explicit handling of this.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D75169