[ARM] Allow Half types in ConstantPool

[SjoerdMeijer]

Summary

Change ARMConstantIslandPass to:

1. accept f16 literals as litpool entries,
2. if the litpool needs to be inserted in the middle of a big block, then we need to 4-byte align the next instruction in ARM mode.

[olista01]

lib/Target/ARM/ARMConstantIslandPass.cpp

513

Is this assertion still useful?

1432

It looks like this function is called when a BB must be split to insert a constant island, but not when extending an existing one. Will this fail if a BB is split to insert a 32-bit constant, and that island is then extended with a 16-bit constant?

Why is this being done for Thumb2?

Would it be easier to always setting the alignment of the new block to 1 for Thumb and 2 for ARM, so that this will work if we add 1-byte literal pools in future? I think that should also fix the issue of extending existing islands too.

[SjoerdMeijer]

Removed the assert, and thanks for the suggestion: I think it is
indeed the easiest and best to always set the alignment.

[olista01]

lib/Target/ARM/ARMConstantIslandPass.cpp

1432

This is still only going to be triggered if the _first_ entry in the constant pool is 2-byte, I think it will fail if the first entry is 4-byte, and the last is 2-byte. Would it also be possible to add a test for this case?

[SjoerdMeijer]

It was indeed failing when there e.g. was an existing 4-byte entry, which was
then followed by a 2-byte entry; the two new ARM and Thumb test cases
showed this. As suggested, we are now always emitting an alignment directive
(I've removed that if-statement).

[SjoerdMeijer]

The Thumb test case was running in an assert related to addressing modes.
I've created D43179 for that, which I need for this patch.

[olista01]

test/CodeGen/ARM/fp16-litpool.ll

47 ↗

(On Diff #133865)

There are more literal pools here than needed for the test, because the addresses of the globals are materialised as literal pools too. Could this be simplified by passing them in as arguments?

This whole test file is also fragile, as it depends on decisions made by other parts of the backend about whether to use literal pools, and the exact ordering of instructions when we get to the constant pool placement pass. It would be better to convert this to an MIR test to avoid that.

[SjoerdMeijer]

Thanks for the suggestions.
I've created MIR tests (and simplified the test case a little bit more).

[olista01]

LGTM, thanks

lib/Target/ARM/ARMConstantIslandPass.cpp

ARMConstantIslands::doInitialConstPlacement(std::vector<MachineInstr*> &CPEMIs) {
 
   // Add all of the constants from the constant pool to the end block, use an
   // identity mapping of CPI's to CPE's.
   const std::vector<MachineConstantPoolEntry> &CPs = MCP->getConstants();
 
   const DataLayout &TD = MF->getDataLayout();
   for (unsigned i = 0, e = CPs.size(); i != e; ++i) {
     unsigned Size = TD.getTypeAllocSize(CPs[i].getType());
-    assert(Size >= 4 && "Too small constant pool entry");
+    assert(Size >= 2 && "Too small constant pool entry");
     unsigned Align = CPs[i].getAlignment();
     assert(isPowerOf2_32(Align) && "Invalid alignment");
     // Verify that all constant pool entries are a multiple of their alignment.
     // If not, we would have to pad them out so that instructions stay aligned.
     assert((Size % Align) == 0 && "CP Entry not multiple of 4 bytes!");
 
     // Insert CONSTPOOL_ENTRY before entries with a smaller alignment.
     unsigned LogAlign = Log2_32(Align);
for (MachineInstr &I : MBB) {
             break;
 
           case ARM::VLDRD:
           case ARM::VLDRS:
             Bits = 8;
             Scale = 4;  // +-(offset_8*4)
             NegOk = true;
             break;
+          case ARM::VLDRH:
+            Bits = 8;
+            Scale = 2;  // +-(offset_8*2)
+            NegOk = true;
+            break;
 
           case ARM::tLDRHi:
             Bits = 5;
             Scale = 2; // +(offset_5*2)
             break;
           }
 
           // Remember that this is a user of a CP entry.
void ARMConstantIslands::createNewWater(unsigned CPUserIndex,
   // We really must not split an IT block.
   DEBUG(unsigned PredReg;
         assert(!isThumb || getITInstrPredicate(*MI, PredReg) == ARMCC::AL));
 
   NewMBB = splitBlockBeforeInstr(&*MI);
 }
 
 /// handleConstantPoolUser - Analyze the specified user, checking to see if it
 /// is out-of-range.  If so, pick up the constant pool value and move it some
 /// place in-range.  Return true if we changed any addresses (thus must run
 /// another pass of branch lengthening), false otherwise.
 bool ARMConstantIslands::handleConstantPoolUser(unsigned CPUserIndex,
                                                 bool CloserWater) {
   CPUser &U = CPUsers[CPUserIndex];
   MachineInstr *UserMI = U.MI;
   MachineInstr *CPEMI  = U.CPEMI;
   unsigned CPI = getCombinedIndex(CPEMI);

test/CodeGen/ARM/fp16-instructions.ll

 ; SOFT:
-; RUN: llc < %s -mtriple=arm-none-eabi -float-abi=soft     | FileCheck %s --check-prefix=CHECK-SOFT
+; RUN: llc < %s -mtriple=arm-none-eabi -float-abi=soft     | FileCheck %s --check-prefixes=CHECK,CHECK-SOFT
 
 ; SOFTFP:
-; RUN: llc < %s -mtriple=arm-none-eabi -mattr=+vfp3        | FileCheck %s --check-prefix=CHECK-SOFTFP-VFP3
-; RUN: llc < %s -mtriple=arm-none-eabi -mattr=+vfp4        | FileCheck %s --check-prefix=CHECK-SOFTFP-FP16
-; RUN: llc < %s -mtriple=arm-none-eabi -mattr=+fullfp16    | FileCheck %s --check-prefix=CHECK-SOFTFP-FULLFP16
+; RUN: llc < %s -mtriple=arm-none-eabi -mattr=+vfp3        | FileCheck %s --check-prefixes=CHECK,CHECK-SOFTFP-VFP3
+; RUN: llc < %s -mtriple=arm-none-eabi -mattr=+vfp4        | FileCheck %s --check-prefixes=CHECK,CHECK-SOFTFP-FP16
+; RUN: llc < %s -mtriple=arm-none-eabi -mattr=+fullfp16    | FileCheck %s --check-prefixes=CHECK,CHECK-SOFTFP-FULLFP16
 
 ; HARD:
-; RUN: llc < %s -mtriple=arm-none-eabihf -mattr=+vfp3      | FileCheck %s --check-prefix=CHECK-HARDFP-VFP3
-; RUN: llc < %s -mtriple=arm-none-eabihf -mattr=+vfp4      | FileCheck %s --check-prefix=CHECK-HARDFP-FP16
-; RUN: llc < %s -mtriple=arm-none-eabihf -mattr=+fullfp16  | FileCheck %s --check-prefix=CHECK-HARDFP-FULLFP16
+; RUN: llc < %s -mtriple=arm-none-eabihf -mattr=+vfp3      | FileCheck %s --check-prefixes=CHECK,CHECK-HARDFP-VFP3
+; RUN: llc < %s -mtriple=arm-none-eabihf -mattr=+vfp4      | FileCheck %s --check-prefixes=CHECK,CHECK-HARDFP-FP16
+; RUN: llc < %s -mtriple=arm-none-eabihf -mattr=+fullfp16  | FileCheck %s --check-prefixes=CHECK,CHECK-HARDFP-FULLFP16
 
 define float @Add(float %a.coerce, float %b.coerce) local_unnamed_addr {
 entry:
   %0 = bitcast float %a.coerce to i32
   %tmp.0.extract.trunc = trunc i32 %0 to i16
   %1 = bitcast i16 %tmp.0.extract.trunc to half
   %2 = bitcast float %b.coerce to i32
   %tmp1.0.extract.trunc = trunc i32 %2 to i16
   %3 = bitcast i16 %tmp1.0.extract.trunc to half
   %add = fadd half %1, %3
   %4 = bitcast half %add to i16
   %tmp4.0.insert.ext = zext i16 %4 to i32
   %5 = bitcast i32 %tmp4.0.insert.ext to float
   ret float %5
 
+; CHECK-LABEL: Add:
+
 ; CHECK-SOFT:  bl  __aeabi_h2f
 ; CHECK-SOFT:  bl  __aeabi_h2f
 ; CHECK-SOFT:  bl  __aeabi_fadd
 ; CHECK-SOFT:  bl  __aeabi_f2h
 
 ; CHECK-SOFTFP-VFP3:  bl  __aeabi_h2f
 ; CHECK-SOFTFP-VFP3:  bl  __aeabi_h2f
 ; CHECK-SOFTFP-VFP3:  vadd.f32

 
 ; CHECK-HARDFP-FP16:  vcvtb.f32.f16 [[S2:s[0-9]]], s1
 ; CHECK-HARDFP-FP16:  vcvtb.f32.f16 [[S0:s[0-9]]], s0
 ; CHECK-HARDFP-FP16:  vadd.f32  [[S0]], [[S0]], [[S2]]
 ; CHECK-HARDFP-FP16:  vcvtb.f16.f32 [[S0]], [[S0]]
 
 ; CHECK-HARDFP-FULLFP16:       vadd.f16  s0, s0, s1
 ; CHECK-HARDFP-FULLFP16-NEXT:  mov pc, lr
+}
 
+define float @AddImm(float %A.coerce) local_unnamed_addr {
+entry:
+  %0 = bitcast float %A.coerce to i32
+  %tmp.0.extract.trunc = trunc i32 %0 to i16
+  %1 = bitcast i16 %tmp.0.extract.trunc to half
+  %add = fadd half %1, 0xH3CF0
+  %2 = bitcast half %add to i16
+  %tmp2.0.insert.ext = zext i16 %2 to i32
+  %3 = bitcast i32 %tmp2.0.insert.ext to float
+  ret float %3
+
+; CHECK-LABEL: AddImm:
+
+; CHECK-HARDFP-FULLFP16:       vldr.16 [[S2:s[0-9]]], .LCPI{{.*}}
+; CHECK-HARDFP-FULLFP16-NEXT:  vadd.f16  s0, s0, [[S2]]
+; CHECK-HARDFP-FULLFP16-NEXT:  mov pc, lr
 }
+