[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

512–513

Is this assertion still useful?

1431

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

1431

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

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) {
   SmallVector<MachineBasicBlock::iterator, 8> InsPoint(MaxAlign + 1, BB->end());
 
   // 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");
     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);
     MachineBasicBlock::iterator InsAt = InsPoint[LogAlign];
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.
if (CC != ARMCC::AL)
       MI = LastIT;
   }
 
   // We really must not split an IT block.
   DEBUG(unsigned PredReg;
         assert(!isThumb || getITInstrPredicate(*MI, PredReg) == ARMCC::AL));
 
   NewMBB = splitBlockBeforeInstr(&*MI);
+
+  // 4 byte align the next block after the constant pool when the CPE is a
+  // 16-bit value in ARM mode, and 2 byte for Thumb.
+  NewMBB->setAlignment(isThumb ? 1 : 2);
 }
 
 /// 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) {

test/CodeGen/ARM/fp16-litpool.ll

+; RUN: llc -mtriple=arm-linux-gnueabihf %s -mattr=+fullfp16 -o - | FileCheck %s --check-prefix=CHECK-ARM
+; RUN: llc -mtriple=thumbv7-linux-gnueabihf %s -mattr=+fullfp16 -o - | FileCheck %s --check-prefix=CHECK-THUMB
+
+; We want to test 2 things here:
+; 1) that f16 literals are accepted 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.
+;
+; These ARM and THUMB test cases have been carefully created using the space
+; intrinsics to trigger the (edge) cases where a 2-byte CP entry is out of
+; range, and inserted directly after an existing CP entry. This checks that we
+; are emitting the correct align directive and that the next instruction is not
+; misaligned.
+
+@LL = common dso_local global i64 0, align 8
+
+declare i32 @llvm.arm.space(i32, i32)
+
+; Function Attrs: minsize nounwind optsize
+define dso_local i32 @ARM(i32 %A.coerce) local_unnamed_addr #0 {
+entry:
+  %S = alloca half, align 2
+  %tmp.0.extract.trunc = trunc i32 %A.coerce to i16
+  %0 = bitcast i16 %tmp.0.extract.trunc to half
+  %S.0.S.0..sroa_cast = bitcast half* %S to i8*
+  store volatile half 0xH3C00, half* %S, align 2
+  store volatile i64 4242424242424242, i64* @LL, align 8
+
+  call i32 @llvm.arm.space(i32 8920, i32 undef)
+
+  %S.0.S.0.570 = load volatile half, half* %S, align 2
+  %add298 = fadd half %S.0.S.0.570, 0xH2E66
+  store volatile half %add298, half* %S, align 2
+
+  call i32 @llvm.arm.space(i32 1350, i32 undef)
+
+  %3 = bitcast half %add298 to i16
+  %tmp343.0.insert.ext = zext i16 %3 to i32
+  ret i32 %tmp343.0.insert.ext
+
+; CHECK-ARM-LABEL: ARM:
+
+; CHECK-ARM:       b .LBB0_{{.}}
+; CHECK-ARM:       .p2align  2
+; CHECK-ARM:      .LCPI0_{{.}}:
+; CHECK-ARM:        .long LL
+; CHECK-ARM:        .p2align  2
+; CHECK-ARM:      .LCPI0_{{.}}:
+; CHECK-ARM:        .long 1576323506
+; CHECK-ARM:        .p2align  2
+; CHECK-ARM:      .LCPI0_{{.}}:
+; CHECK-ARM:        .long 987766
+; CHECK-ARM:        .p2align  1
+; CHECK-ARM:      .LCPI0_{{.}}:
+; CHECK-ARM:        .short  11878
+; CHECK-ARM:        .p2align  2
+; CHECK-ARM:      .LBB0_{{.}}:
+; CHECK-ARM:        .zero 8920
+}
+
+define dso_local i32 @THUMB(i32 %A.coerce) local_unnamed_addr #1 {
+entry:
+  %F = alloca float, align 4
+  %S = alloca half, align 2
+  %tmp.0.extract.trunc = trunc i32 %A.coerce to i16
+  %0 = bitcast i16 %tmp.0.extract.trunc to half
+  %F.0.F.0..sroa_cast = bitcast float* %F to i8*
+  store volatile float 4.200000e+01, float* %F, align 4
+  %S.0.S.0..sroa_cast = bitcast half* %S to i8*
+  store volatile half 0xH3C00, half* %S, align 2
+  %S.0.S.0.142 = load volatile half, half* %S, align 2
+
+  call i32 @llvm.arm.space(i32 1230, i32 undef)
+
+  %add42 = fadd half %S.0.S.0.142, 0xH2E66
+  store volatile half %add42, half* %S, align 2
+
+  call i32 @llvm.arm.space(i32 1330, i32 undef)
+
+  %S.0.S.0.119 = load volatile half, half* %S, align 2
+  %3 = bitcast half %add42 to i16
+  %tmp87.0.insert.ext = zext i16 %3 to i32
+  ret i32 %tmp87.0.insert.ext
+
+; CHECK-THUMB-LABEL: THUMB:
+
+; CHECK-THUMB:   b.w .LBB1_{{.}}
+; CHECK-THUMB:   .p2align  2
+; CHECK-THUMB: .LCPI{{.*}}:
+; CHECK-THUMB:   .long 1109917696
+; CHECK-THUMB:   .p2align  1
+; CHECK-THUMB: .LCPI{{.*}}:
+; CHECK-THUMB:   .short  11878
+; CHECK-THUMB:   .p2align  1
+; CHECK-THUMB: .LBB1_{{.}}:
+; CHECK-THUMB:   .zero 1230
+
+}
+
+attributes #0 = { minsize nounwind optsize "target-features"="+crc,+crypto,+dsp,+fp-armv8,+fullfp16,+hwdiv,+hwdiv-arm,+neon,+ras,+strict-align,-thumb-mode" }
+attributes #1 = { minsize nounwind optsize "target-features"="+crc,+crypto,+dsp,+fp-armv8,+fullfp16,+hwdiv,+hwdiv-arm,+neon,+ras,+strict-align,+thumb-mode" }