[AArch64] Fix the upper limit for folded address offsets for COFF

[mstorsjo]

Summary

In COFF, the immediates in IMAGE_REL_ARM64_PAGEBASE_REL21 relocations are limited to 21 bit signed, i.e. the offset has to be less than
(1 << 20). The previous limit did intend to cover for this case, but had missed that the 21 bit field was signed.

This fixes issue https://github.com/llvm/llvm-project/issues/54753.

[efriedma]

Please add a testcase.

Do you agree we should make this dependent on the target OS (or more precisely, object file format), or should we just reduce the universal limit and keep it object file format agnostic?

I'd like to keep the limits of specific formats documented, so future readers can understand why the specific number was chosen. I don't really care whether that's code or a comment; it's unlikely to matter much in practice.

I'm not actually sure where "1 << 21" comes from, though; I think ELF is unlimited, and MachO is limited to "1 << 23".

[pcc]

IIRC 1 << 21 was from COFF (missed that the addend was signed), so it wouldn't make sense to have this be 1 << 21 on other object formats if the actual limit is higher.

I think it's unlikely that people will be hitting this limit in practice, so I'd probably just change this to 20 and document that it comes from COFF. I don't think this needs to be conditional but if it were it should be based on the object format and not the operating system because there may be non-Windows COFF users (UEFI comes to mind).

[mstorsjo]

In D123160#3430996, @efriedma wrote:

I'm not actually sure where "1 << 21" comes from, though; I think ELF is unlimited, and MachO is limited to "1 << 23".

In D123160#3431163, @pcc wrote:

IIRC 1 << 21 was from COFF (missed that the addend was signed), so it wouldn't make sense to have this be 1 << 21 on other object formats if the actual limit is higher.

I think it's unlikely that people will be hitting this limit in practice, so I'd probably just change this to 20 and document that it comes from COFF. I don't think this needs to be conditional but if it were it should be based on the object format and not the operating system because there may be non-Windows COFF users (UEFI comes to mind).

Awesome, thanks - that clarifies the existing intent and makes it clearer what kind of test to add for this (just adjusting the existing one).

[mstorsjo]

Updated the patch to only adjust the existing universal limit from 1 << 21 to 1 << 20, and clarified the comment to explain why this limit is chosen. Updated the existing testcase which tests the boundary conditions of this limit.

[efriedma]

LGTM

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp

static SDValue performGlobalAddressCombine(SDNode *N, SelectionDAG &DAG,
   // Require that the new offset is larger than the existing one. Otherwise, we
   // can end up oscillating between two possible DAGs, for example,
   // (add (add globaladdr + 10, -1), 1) and (add globaladdr + 9, 1).
   if (Offset <= uint64_t(GN->getOffset()))
     return SDValue();
 
   // Check whether folding this offset is legal. It must not go out of bounds of
   // the referenced object to avoid violating the code model, and must be
-  // smaller than 2^21 because this is the largest offset expressible in all
-  // object formats.
+  // smaller than 2^20 because this is the largest offset expressible in all
+  // object formats. (The IMAGE_REL_ARM64_PAGEBASE_REL21 relocation in COFF
+  // stores an immediate signed 21 bit offset.)
   //
   // This check also prevents us from folding negative offsets, which will end
   // up being treated in the same way as large positive ones. They could also
   // cause code model violations, and aren't really common enough to matter.
-  if (Offset >= (1 << 21))
+  if (Offset >= (1 << 20))
     return SDValue();
 
   const GlobalValue *GV = GN->getGlobal();
   Type *T = GV->getValueType();
   if (!T->isSized() ||
       Offset > GV->getParent()->getDataLayout().getTypeAllocSize(T))
     return SDValue();
 

llvm/lib/Target/AArch64/GISel/AArch64PreLegalizerCombiner.cpp

static bool matchFoldGlobalOffset(MachineInstr &MI, MachineRegisterInfo &MRI,
   // infinite loops.
   uint64_t CurrOffset = GlobalOp.getOffset();
   uint64_t NewOffset = MinOffset + CurrOffset;
   if (NewOffset <= CurrOffset)
     return false;
 
   // Check whether folding this offset is legal. It must not go out of bounds of
   // the referenced object to avoid violating the code model, and must be
-  // smaller than 2^21 because this is the largest offset expressible in all
-  // object formats.
+  // smaller than 2^20 because this is the largest offset expressible in all
+  // object formats. (The IMAGE_REL_ARM64_PAGEBASE_REL21 relocation in COFF
+  // stores an immediate signed 21 bit offset.)
   //
   // This check also prevents us from folding negative offsets, which will end
   // up being treated in the same way as large positive ones. They could also
   // cause code model violations, and aren't really common enough to matter.
-  if (NewOffset >= (1 << 21))
+  if (NewOffset >= (1 << 20))
     return false;
 
   Type *T = GV->getValueType();
   if (!T->isSized() ||
       NewOffset > GV->getParent()->getDataLayout().getTypeAllocSize(T))
     return false;
   MatchInfo = std::make_pair(NewOffset, MinOffset);
   return true;

llvm/test/CodeGen/AArch64/fold-global-offsets.ll

 ; GISEL-NEXT:    ret
   %l = load [2 x i64], [2 x i64]* bitcast (i8* getelementptr (i8, i8* bitcast ([16777216 x i64]* @x2 to i8*), i64 8) to [2 x i64]*)
   ret [2 x i64] %l
 }
 
 define i64 @f5() {
 ; CHECK-LABEL: f5:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    adrp x8, x2+2097144
-; CHECK-NEXT:    ldr x0, [x8, :lo12:x2+2097144]
+; CHECK-NEXT:    adrp x8, x2+1048568
+; CHECK-NEXT:    ldr x0, [x8, :lo12:x2+1048568]
 ; CHECK-NEXT:    ret
 ;
 ; GISEL-LABEL: f5:
 ; GISEL:       // %bb.0:
-; GISEL-NEXT:    adrp x8, x2+2097144
-; GISEL-NEXT:    ldr x0, [x8, :lo12:x2+2097144]
+; GISEL-NEXT:    adrp x8, x2+1048568
+; GISEL-NEXT:    ldr x0, [x8, :lo12:x2+1048568]
 ; GISEL-NEXT:    ret
-  %l = load i64, i64* getelementptr ([16777216 x i64], [16777216 x i64]* @x2, i64 0, i64 262143)
+  %l = load i64, i64* getelementptr ([16777216 x i64], [16777216 x i64]* @x2, i64 0, i64 131071)
   ret i64 %l
 }
 
 define i64 @f6() {
 ; CHECK-LABEL: f6:
 ; CHECK:       // %bb.0:
-; CHECK-NEXT:    mov w8, #2097152
+; CHECK-NEXT:    mov w8, #1048576
 ; CHECK-NEXT:    adrp x9, x2
 ; CHECK-NEXT:    add x9, x9, :lo12:x2
 ; CHECK-NEXT:    ldr x0, [x9, x8]
 ; CHECK-NEXT:    ret
 ;
 ; GISEL-LABEL: f6:
 ; GISEL:       // %bb.0:
-; GISEL-NEXT:    mov w8, #2097152
+; GISEL-NEXT:    mov w8, #1048576
 ; GISEL-NEXT:    adrp x9, x2
 ; GISEL-NEXT:    add x9, x9, :lo12:x2
 ; GISEL-NEXT:    ldr x0, [x9, x8]
 ; GISEL-NEXT:    ret
-  %l = load i64, i64* getelementptr ([16777216 x i64], [16777216 x i64]* @x2, i64 0, i64 262144)
+  %l = load i64, i64* getelementptr ([16777216 x i64], [16777216 x i64]* @x2, i64 0, i64 131072)
   ret i64 %l
 }
 
 define i32 @f7() {
 ; FIXME: GlobalISel doesn't handle vectors well.
 ;
 ; CHECK-LABEL: f7:
 ; CHECK:       // %bb.0: // %entry