[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 ↗

(On Diff #133611)

Is this assertion still useful?

1432 ↗

(On Diff #133611)

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 ↗

(On Diff #133611)

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

llvm/trunk/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");
     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.
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) {

llvm/trunk/test/CodeGen/ARM/fp16-litpool-arm.mir

+# RUN: llc -mtriple=arm-none-eabi -run-pass=arm-cp-islands %s -o - | FileCheck %s
+
+--- |
+  ; ModuleID = '<stdin>'
+  source_filename = "<stdin>"
+  target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+  target triple = "arm-arm--eabi"
+
+  declare i32 @llvm.arm.space(i32, i32) #0
+
+  define dso_local i32 @ARM(i64* %LL, i32 %A.coerce) local_unnamed_addr #1 {
+  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
+    store volatile half 0xH3C00, half* %S, align 2
+    store volatile i64 4242424242424242, i64* %LL, align 8
+    %1 = 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
+    %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
+  }
+
+  attributes #0 = { nounwind }
+  attributes #1 = { minsize nounwind optsize "target-features"="+crc,+crypto,+dsp,+fp-armv8,+fullfp16,+hwdiv,+hwdiv-arm,+neon,+ras,+strict-align,-thumb-mode" }
+
+...
+---
+name:            ARM
+alignment:       2
+tracksRegLiveness: true
+liveins:
+  - { reg: '$r0' }
+frameInfo:
+  stackSize:       4
+  maxAlignment:    2
+  maxCallFrameSize: 0
+stack:
+  - { id: 0, name: S, offset: -2, size: 2, alignment: 2, stack-id: 0, local-offset: -2 }
+constants:
+  - id:              0
+    value:           i32 1576323506
+    alignment:       4
+  - id:              1
+    value:           i32 987766
+    alignment:       4
+  - id:              2
+    value:           half 0xH2E66
+    alignment:       2
+
+#CHECK:  B %[[BB4:bb.[0-9]]]
+#CHECK:  bb.{{.}} (align 2):
+#CHECK:    successors:
+#CHECK:    CONSTPOOL_ENTRY {{.}}, %const.{{.}}, 4
+#CHECK:  bb.{{.}} (align 2):
+#CHECK:    successors:
+#CHECK:    CONSTPOOL_ENTRY {{.}}, %const.{{.}}, 4
+#CHECK:  bb.{{.}} (align 1):
+#CHECK:    successors:
+#CHECK:    CONSTPOOL_ENTRY {{.}}, %const.{{.}}, 2
+#CHECK:  [[BB4]].entry (align 2):
+
+body:             |
+  bb.0.entry:
+    liveins: $r0
+
+    $sp = frame-setup SUBri $sp, 4, 14, $noreg, $noreg
+    frame-setup CFI_INSTRUCTION def_cfa_offset 4
+    renamable $s0 = FCONSTH 112, 14, $noreg
+    renamable $r1 = LDRcp %const.0, 0, 14, $noreg :: (load 4 from constant-pool)
+    renamable $r2 = LDRcp %const.1, 0, 14, $noreg :: (load 4 from constant-pool)
+    VSTRH killed renamable $s0, $sp, 1, 14, $noreg :: (volatile store 2 into %ir.S)
+    STRi12 killed renamable $r2, renamable $r0, 4, 14, $noreg :: (volatile store 4 into %ir.LL + 4)
+    renamable $s0 = VLDRH %const.2, 0, 14, $noreg :: (load 2 from constant-pool)
+    STRi12 killed renamable $r1, killed renamable $r0, 0, 14, $noreg :: (volatile store 4 into %ir.LL, align 8)
+    dead renamable $r0 = SPACE 8920, undef renamable $r0
+    renamable $s2 = VLDRH $sp, 1, 14, $noreg :: (volatile dereferenceable load 2 from %ir.S)
+    renamable $s0 = VADDH killed renamable $s2, killed renamable $s0, 14, $noreg
+    VSTRH renamable $s0, $sp, 1, 14, $noreg :: (volatile store 2 into %ir.S)
+    renamable $r0 = VMOVRH killed renamable $s0, 14, $noreg
+    dead renamable $r1 = SPACE 1350, undef renamable $r0
+    $sp = ADDri $sp, 4, 14, $noreg, $noreg
+    MOVPCLR 14, $noreg, implicit killed $r0
+
+...

llvm/trunk/test/CodeGen/ARM/fp16-litpool-thumb.mir

+# RUN: llc -mtriple=thumbv7-none-eabi -run-pass=arm-cp-islands %s -o - | FileCheck %s
+--- |
+  ; ModuleID = '<stdin>'
+  source_filename = "<stdin>"
+  target datalayout = "e-m:e-p:32:32-i64:64-v128:64:128-a:0:32-n32-S64"
+  target triple = "thumbv7-arm--eabi"
+
+  declare i32 @llvm.arm.space(i32, i32) #0
+
+  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
+    store volatile float 4.200000e+01, float* %F, align 4
+    store volatile half 0xH3C00, half* %S, align 2
+    %S.0.S.0.142 = load volatile half, half* %S, align 2
+    %1 = 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
+    %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
+  }
+
+  attributes #0 = { nounwind }
+  attributes #1 = { minsize nounwind optsize "target-features"="+crc,+crypto,+dsp,+fp-armv8,+fullfp16,+hwdiv,+hwdiv-arm,+neon,+ras,+strict-align,+thumb-mode" }
+
+...
+---
+name:            THUMB
+alignment:       1
+tracksRegLiveness: true
+frameInfo:
+  stackSize:       8
+  maxAlignment:    4
+  maxCallFrameSize: 0
+stack:
+  - { id: 0, name: F, offset: -4, size: 4, alignment: 4, stack-id: 0, local-offset: -4 }
+  - { id: 1, name: S, offset: -6, size: 2, alignment: 2, stack-id: 0, local-offset: -6 }
+constants:
+  - id:              0
+    value:           i32 1109917696
+    alignment:       4
+  - id:              1
+    value:           half 0xH2E66
+    alignment:       2
+
+#CHECK:  t2B %[[BB3:bb.[0-9]]]
+#CHECK:  bb.{{.}} (align 2):
+#CHECK:    successors:
+#CHECK:    CONSTPOOL_ENTRY 2, %const.{{.}}, 4
+#CHECK:  bb.{{.}} (align 1):
+#CHECK:    successors:
+#CHECK:    CONSTPOOL_ENTRY 3, %const.{{.}}, 2
+#CHECK:  [[BB3]].entry (align 1):
+
+body:             |
+  bb.0.entry:
+    $sp = frame-setup tSUBspi $sp, 2, 14, $noreg
+    frame-setup CFI_INSTRUCTION def_cfa_offset 8
+    renamable $r0 = tLDRpci %const.0, 14, $noreg :: (load 4 from constant-pool)
+    renamable $s0 = FCONSTH 112, 14, $noreg
+    tSTRspi killed renamable $r0, $sp, 1, 14, $noreg :: (volatile store 4 into %ir.F)
+    VSTRH killed renamable $s0, $sp, 1, 14, $noreg :: (volatile store 2 into %ir.S)
+    renamable $s2 = VLDRH $sp, 1, 14, $noreg :: (volatile dereferenceable load 2 from %ir.S)
+    renamable $s0 = VLDRH %const.1, 0, 14, $noreg :: (load 2 from constant-pool)
+    dead renamable $r0 = SPACE 1230, undef renamable $r0
+    renamable $s0 = VADDH killed renamable $s2, killed renamable $s0, 14, $noreg
+    VSTRH renamable $s0, $sp, 1, 14, $noreg :: (volatile store 2 into %ir.S)
+    renamable $r0 = VMOVRH killed renamable $s0, 14, $noreg
+    dead renamable $r1 = SPACE 1330, undef renamable $r0
+    dead renamable $s0 = VLDRH $sp, 1, 14, $noreg :: (volatile dereferenceable load 2 from %ir.S)
+    $sp = tADDspi $sp, 2, 14, $noreg
+    tBX_RET 14, $noreg, implicit killed $r0
+
+...