[PATCH] D38315: [ARM] Armv8.2-A FP16 code generation (part 1/2)

[Sjoerd Meijer via Phabricator via llvm-commits]

SjoerdMeijer added inline comments.


================
Comment at: lib/Target/ARM/ARMCallingConv.td:159
 
+  CCIfType<[f16], CCBitConvertToType<i16>>,
+
----------------
olista01 wrote:
> Are the changes in this file needed if we're still using the clang hack for the calling convention?
No, you're right, we don't need it. I will remove it.


================
Comment at: lib/Target/ARM/ARMISelLowering.cpp:2485
+
+    if (Subtarget->hasFullFP16() && Subtarget->isTargetHardFloat()) {
+      // Half-precision return values can be returned like this:
----------------
olista01 wrote:
> I think it would be better to fix the code-generation for bitcasts generally, rather than putting in these special cases for arguments and returns. Also, this doesn't seem to be working in one of your test cases (we get store/load pairs), do you know why?
I wanted to do this fix up in ExpandBITCAST as well, like I do for the f16 function arguments, but I simply was not able to get it working. Recognising the pattern is easy, but fixing up the subsequent CopyToReg and the ARMISD::RET_FLAG nodes, i.e. replace uses, chains, glues, for these nodes was such a pain and while the rewrite looked okay, I kept running in very late segfaults because there was some funny state left. I think doing the rewrite here is equally fine: it is straightforward and we get generation of the CopyToReg and return nodes for free by this simple rewrite here, the DAG replaces we would have to do are a lot more ugly I think.

This rewrite has to be done here, or in ExpandBITCAST, because otherwise it is too late. Very early in the DAG creation and  in the legalizer, this gets legalized to stack Stores and loads. Thus, tablegen rewrite rules to rewrite "bitcasts" is not going to help, because long before we do instruction selection, these bitconverts no longer exist.

I will look into the loads/stores that you mentioned.


================
Comment at: lib/Target/ARM/MCTargetDesc/ARMBaseInfo.h:273
     // This four-bit field describes the addressing mode used.
-    AddrModeMask  = 0x1f, // The AddrMode enums are declared in ARMBaseInfo.h
+    AddrModeMask  = 0x3f, // The AddrMode enums are declared in ARMBaseInfo.h
 
----------------
olista01 wrote:
> Why is this needed? The new value 17 still fits in 5 bits. If this is actually needed, then do the *Shift values below need to be increased?
Thanks, will fix.


https://reviews.llvm.org/D38315