[ARM] Fix so immediates and pc relative checks

[simonwallis2]

Summary

Treating an SoImm offset as a multiple of 4 between -1020 and 1020
mis-handles the second of a pair of 16-bit constants where the offset is a multiple of 2 but not a multiple of 4,
leading to an LLVM ERROR: out of range pc-relative fixup value

For 32-bit and larger (64-bit) constants, continue to treat an SoImm offset as a multiple of 4 between -1020 and 1020.
For smaller (16-bit) constants, treat an SoImm offset as a multiple of 1 between -255 and 255.

[efriedma]

llvm/lib/Target/ARM/ARMConstantIslandPass.cpp

779

Standard code style in LLVM is something like case ARM::LEApcrelJT: {

781

Does this do something sane for ARM::LEApcrelJT? I don't think the operand for that is a CPI.

789

Instead of checking "if (LogCPEAlign >= 2)", could we just set "Scale = CPEAlign;"? I don't care that much about the quality of lowering for 16-bit constant pool entries, but the intent would be more clear.

[simonwallis2]

After review feedback: made some formatting changes in line with standard code style in LLVM.

Added new test variant to highlight a case raised in other review feedback.

[simonwallis2]

llvm/lib/Target/ARM/ARMConstantIslandPass.cpp

779

Thanks.
Done.

781

I have checked a number of arm::LEApcrelJT cases in the test suite
and it does appear to do something sane.

789

No, we could not just set Scale = CPEAlign.

Setting Scale = CPEAlign would cause
LLVM ERROR: out of range pc-relative fixup value
for various of offsets. I have added one such case to constant-island-soimm-limit16.mir.

It would also change the behaviour for 64-bit constants.

[efriedma]

llvm/lib/Target/ARM/ARMConstantIslandPass.cpp

781

Okay.

789

Oh, I see, the right expression would be something like Scale = 1 << (LogCPEAlign & ~1);. Which is probably overkill.

Maybe clarify the comment a bit? The "which is safe for 2-byte constants" is a little misleading; probably better to just say "constants with any alignment".

[simonwallis2]

llvm/lib/Target/ARM/ARMConstantIslandPass.cpp

789

Oh, I see, the right expression would be something like Scale = 1 << (LogCPEAlign & ~1);. Which is probably overkill.

In order to represent a reachable range of contiguous offsets, the value of Scale must be all of:

• An even power of 2 for a valid SoImm encoding.
• Not less than the alignment of the constant.
• Not more than the instruction width in bytes (4).

So the only permitted values are 1 or 4.

Computing 1<<(LogCPEAlign & ~1) would violate this if constants with alignment > 8 bytes were added.

789

Maybe clarify the comment a bit? The "which is safe for 2-byte constants" is a little misleading; probably better to just say "constants with any alignment".

How about changing

// We'll pretend the maximum offset is 255 * 1,
// which is safe for 2-byte constants with 2-byte alignment.

to

// We'll pretend the maximum offset is 255 * 1,
// which is safe for constants with less than 4-byte alignment.

[simonwallis2]

Reworded a comment. NFC.

[simonwallis2]

Ping.

[efriedma]

LGTM

llvm/lib/Target/ARM/ARMConstantIslandPass.cpp

for (MachineInstr &I : MBB) {
           bool IsSoImm = false;
 
           switch (Opc) {
           default:
             llvm_unreachable("Unknown addressing mode for CP reference!");
 
           // Taking the address of a CP entry.
           case ARM::LEApcrel:
-          case ARM::LEApcrelJT:
+          case ARM::LEApcrelJT: {
               // This takes a SoImm, which is 8 bit immediate rotated. We'll
               // pretend the maximum offset is 255 * 4. Since each instruction
               // 4 byte wide, this is always correct. We'll check for other
               // displacements that fits in a SoImm as well.
               Bits = 8;
-            Scale = 4;
               NegOk = true;
               IsSoImm = true;
+              unsigned CPI = I.getOperand(op).getIndex();
+              MachineInstr *CPEMI = CPEMIs[CPI];
+              const Align CPEAlign = getCPEAlign(CPEMI);
+              const unsigned LogCPEAlign = Log2(CPEAlign);
+              if (LogCPEAlign >= 2)
+                Scale = 4;
+              else
+                // For constants with less than 4-byte alignment,
+                // we'll pretend the maximum offset is 255 * 1.
+                Scale = 1;
+            }
             break;
           case ARM::t2LEApcrel:
           case ARM::t2LEApcrelJT:
             Bits = 12;
             NegOk = true;
             break;
           case ARM::tLEApcrel:
           case ARM::tLEApcrelJT:

llvm/test/CodeGen/ARM/constant-island-SOImm-limit16.mir

+# RUN: sed -e "s/SPACEBYTES/100/g" %s | sed -e "s/OFFSET/116/g" > %t.mir
+# RUN: llc %t.mir --filetype=obj -start-before=arm-cp-islands -o - | \
+# RUN: llvm-objdump --arch=armv8a --disassemble - | FileCheck %t.mir
+
+# RUN: sed -e "s/SPACEBYTES/400/g" %s | sed -e "s/OFFSET/12/g" > %t.mir
+# RUN: llc %t.mir --filetype=obj -start-before=arm-cp-islands -o - | \
+# RUN: llvm-objdump --arch=armv8a --disassemble - | FileCheck %t.mir
+
+# RUN: sed -e "s/SPACEBYTES/800/g" %s | sed -e "s/OFFSET/12/g" > %t.mir
+# RUN: llc %t.mir --filetype=obj -start-before=arm-cp-islands -o - | \
+# RUN: llvm-objdump --arch=armv8a --disassemble - | FileCheck %t.mir
+
+--- |
+  target triple = "armv8.2a-arm-none-eabi"
+
+  define dso_local i32 @main() #0 { ret i32 0 }
+
+  attributes #0 = { "frame-pointer"="all" } !4 = !{i32 210}
+
+...
+---
+
+name:            main
+alignment:       4
+tracksRegLiveness: true
+constants:
+
+-
+ id:              0
+ value:           half 0xH5440
+ alignment:       2
+-
+ id:              1
+ value:           half 0xH5441
+ alignment:       2
+
+machineFunctionInfo: {}
+body:             |
+
+  bb.0 (%ir-block.0):
+    liveins: $lr
+
+    $sp = frame-setup STMDB_UPD $sp, 14, $noreg, killed $r11, killed $lr
+    $r11 = frame-setup MOVr killed $sp, 14, $noreg, $noreg
+    $sp = frame-setup SUBri killed $sp, 80, 14, $noreg, $noreg
+
+    ; Test handling of 16-bit constant pool entries.
+    ; 2 consecutive entries: 1 is 4-byte aligned, 1 is not 4-byte aligned.
+
+    renamable $r1 = LEApcrel %const.0, 14, $noreg
+    renamable $r1 = LDRH killed renamable $r1, $noreg, 0, 14, $noreg :: (load 2 from constant-pool)
+    renamable $r1 = LEApcrel %const.1, 14, $noreg
+    renamable $r1 = LDRH killed renamable $r1, $noreg, 0, 14, $noreg :: (load 2 from constant-pool)
+
+    renamable $r0 = SPACE SPACEBYTES, undef renamable $r0
+
+    $sp = frame-destroy MOVr $r11, 14, $noreg, $noreg
+    $sp = frame-destroy LDMIA_RET $sp, 14, $noreg, def $r11, def $pc, implicit killed $r0
+
+ # CHECK: add r1, pc, #OFFSET
+---
+...