[AsmPrinter] Handle foldable FP constant exprs in initializers.

[fhahn]

Summary

Currently LLVM crashes on the added tests, because it tries to create a
MCConstantExpr for floating point constants.

The constant expressions in the example are not folded at IR generation
time and require ConstantFoldConstant to fold them. The patch updates
emitGlobalConstantImpl to use emitGlobalConstant for floating point
constant expressions that can be folded to a ConstantFP.

[arsenm]

llvm/test/CodeGen/AArch64/complex-float-constexpr-global-initializer.ll

10–11

Could use a case where the folded FP op is a vector

[fhahn]

Add extract directly from vector without bitcast

[fhahn]

llvm/test/CodeGen/AArch64/complex-float-constexpr-global-initializer.ll

10–11

I've updated the test to extract one element directly from a vector instead of going through a bit cast. Is that along the lines you suggested?

[arsenm]

llvm/test/CodeGen/AArch64/complex-float-constexpr-global-initializer.ll

10–11

Not quite, this is still folding a pair of scalar FP operation on values that happen to be extracted from a vector. Folding a vector FP operation should also work

[fhahn]

Add constant with fadd constexpr.

[fhahn]

llvm/test/CodeGen/AArch64/complex-float-constexpr-global-initializer.ll

10–11

Ah right. I've added a fadd constexpr.

[Gerolf]

It looks a bit weird to check for FP constant again (see line 2802). The crux is that constant folding for CE fires only when size > 8 on line 2825. If folding happened at this point independent of size, then you would not need your checks in 2840ff. So could the size check be simply removed for the fix?

[efriedma]

If the frontend is generating constants that require calling ConstantFoldConstant to put them into an emittable form, I think something has gone wrong else. I mean, I guess there's no real harm in trying to perform this fold as a last-ditch effort, but I'm not convinced there's a bug in the backend.

[fhahn]

To provide some context, in the Clang crash that surfaced this, the constant actually comes from aarch64-promote-const. I guess in most cases, it's not an issue in practice, as ConstantExpr will do simple folds at construction time, but not the bit cast one in the example.

In D79775#2032079, @efriedma wrote:

If the frontend is generating constants that require calling ConstantFoldConstant to put them into an emittable form, I think something has gone wrong else. I mean, I guess there's no real harm in trying to perform this fold as a last-ditch effort, but I'm not convinced there's a bug in the backend.

Hm, IIUC AsmPrinter::lowerConstant tries to traverse complex constant expressions and constructs MCExprs for them, to lower them in multiple steps, if required, also involving ConstantFoldConstant. The problem is that MCConstantExpr does not support ConstantFP values and once a a floating point constant like 0.0 is hit while traversing a constant expression, the AsmPrinter crashes.

I am not sure, is there a reason I am missing why complex integer constant expressions can be emitted, but not floating point ones? Maybe an alternative fix would be to support ConstantFP in MCConstantExpr?

[efriedma]

the constant actually comes from aarch64-promote-const

That looks like a bug: aarch64-promote-constant is sticking constants in global variables without checking whether we can actually lower them. Probably it should be calling Constant::needsRelocation() or something like that.

[fhahn]

In D79775#2032401, @efriedma wrote:

the constant actually comes from aarch64-promote-const

That looks like a bug: aarch64-promote-constant is sticking constants in global variables without checking whether we can actually lower them. Probably it should be calling Constant::needsRelocation() or something like that.

For the constants causing the crash here, relocations are not a problem, just the fact that AsmPrinter does not support emitting ConstantFP in ConstantExprs. I've put up D79775, which skips promoting such constants. As a next step, it might make sense to use ConstantFoldConstant there to allow promoting constantexpr that can be completely folded. What do you think? I think it might be still reasonable to support emitting fold-able ConstantExprs in AsmPrinter, but it's a more invasive change and we should probably first go with the fix in AArch64PromoteConstant.

With respect to relocations, that might be an independent issue. For example, the IR below will result in emitting references to constants _g without relocations.

@g = external global i64
define [2 x i64] @test3() {
  ret [2 x i64] [i64 add (i64 ptrtoint (i64* @g to i64), i64 ptrtoint (i64* @g to i64)), i64 ptrtoint (i64* @g to i64)]
}


; %bb.0:
Lloh4:
	adrp	x8, __PromotedConst@PAGE
Lloh5:
	add	x8, x8, __PromotedConst@PAGEOFF
	ldp	x0, x1, [x8]
	ret
	.loh AdrpAdd	Lloh4, Lloh5
	.cfi_endproc
                                        ; -- End function
	.section	__DATA,__const
	.p2align	3               ; @_PromotedConst
__PromotedConst:
	.quad	_g+_g
	.quad	_g

[efriedma]

needsRelocation() includes both actual references to globals, and arbitrary constantexprs that can't be emitted in any straightforward manner.

Granted, in theory, if a constant expression doesn't refer to any globals, it should theoretically be possible to fold it down to a form the asmprinter can emit: there isn't anything that can vary, so there's only one possible result. And maybe our constant folder is in fact powerful enough to do that? But I'd rather be conservative, and not assume that. So I'd rather just restrict the form of the input to the backend: no ConstantExprs, except for a few specific forms of expressions involving pointers which we know the AsmPrinter supports.

[fhahn]

In D79775#2034437, @efriedma wrote:

needsRelocation() includes both actual references to globals, and arbitrary constantexprs that can't be emitted in any straightforward manner.

Hm, the version I am looking at seems to only return true for GlobalValues & block addresses in functions needing relocations. https://github.com/llvm/llvm-project/blob/master/llvm/lib/IR/Constants.cpp#L553

Granted, in theory, if a constant expression doesn't refer to any globals, it should theoretically be possible to fold it down to a form the asmprinter can emit: there isn't anything that can vary, so there's only one possible result. And maybe our constant folder is in fact powerful enough to do that? But I'd rather be conservative, and not assume that. So I'd rather just restrict the form of the input to the backend: no ConstantExprs, except for a few specific forms of expressions involving pointers which we know the AsmPrinter supports.

Sounds good to me! D79775 specifically rejects floating point constant expressions in AArch64PromoteConstant. I think rejecting expressions requiring relocations/other complex exprs would be best done as follow-ups.

[efriedma]

Hm, the version I am looking at seems to only return true for GlobalValues & block addresses in functions needing relocations. https://github.com/llvm/llvm-project/blob/master/llvm/lib/IR/Constants.cpp#L553

Oh, hmm, somehow I thought it was different.

We probably need some common helper that actually does what we want, somewhere, so we don't have duplicate code for figuring out whether it's legal to promote a constant to a global.

[fhahn]

In D79775#2034646, @efriedma wrote:

Hm, the version I am looking at seems to only return true for GlobalValues & block addresses in functions needing relocations. https://github.com/llvm/llvm-project/blob/master/llvm/lib/IR/Constants.cpp#L553

Oh, hmm, somehow I thought it was different.

We probably need some common helper that actually does what we want, somewhere, so we don't have duplicate code for figuring out whether it's legal to promote a constant to a global.

Thanks for pointing me in the right direction! I've submitted a fix directly to AArch64PromoteConstant (824a8593328c) rather than modifying AsmPrinter. I'll potentially follow up on the AArch64PromoteConstant patch with another one that tries to simplify constant expressions before deciding in the pass.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

if (Size > 8) {
       if (New != CE)
         return emitGlobalConstantImpl(DL, New, AP);
     }
   }
 
   if (const ConstantVector *V = dyn_cast<ConstantVector>(CV))
     return emitGlobalConstantVector(DL, V, AP);
 
-  // Otherwise, it must be a ConstantExpr.  Lower it to an MCExpr, then emit it
-  // thread the streamer with EmitValue.
+  // Otherwise, it must be a ConstantExpr.
+
+  // First, try to constant fold floating point constant expressions to a FP
+  // constant. Those cannot be lowered to MCExpr.
+  if (CV->getType()->isFloatingPointTy()) {
+    ConstantFP *New = cast<ConstantFP>(ConstantFoldConstant(CV, DL));
+    if (New)
+      return emitGlobalConstantFP(New, AP);
+  }
+
+  // Lower it to an MCExpr, then emit it thread the streamer with EmitValue.
   const MCExpr *ME = AP.lowerConstant(CV);
 
   // Since lowerConstant already folded and got rid of all IR pointer and
   // integer casts, detect GOT equivalent accesses by looking into the MCExpr
   // directly.
   if (AP.getObjFileLowering().supportIndirectSymViaGOTPCRel())
     handleIndirectSymViaGOTPCRel(AP, &ME, BaseCV, Offset);
 

llvm/test/CodeGen/AArch64/complex-float-constexpr-global-initializer.ll

+; RUN: llc -o - %s | FileCheck %s
+
+target datalayout = "e-m:o-i64:64-i128:128-n32:64-S128"
+target triple = "arm64-apple-ios13.0.0"
+
+; Make sure complex floating point constant expressions are lowered properly.
+
+@g = global [1 x <2 x float>] [<2 x float>
+      <float fneg (float extractelement (<4 x float> bitcast (<1 x i128> <i128 1> to <4 x float>), i32 0)),
+       float fneg (float extractelement (<4 x float> <float 0.0, float 10.5, float 0.0, float 0.0>, i32 1))>]
+
+define [1 x <2 x float>] @load1() {
+; CHECK-LABEL: _load1:
+; CHECK-NEXT:    .cfi_startproc
+; CHECK-NEXT: ; %bb.0:
+; CHECK-NEXT: Lloh0:
+; CHECK-NEXT:    adrp    x8, _g@PAGE
+; CHECK-NEXT: Lloh1:
+; CHECK-NEXT:    ldr d0, [x8, _g@PAGEOFF]
+; CHECK-NEXT:    ret
+
+  %r = load [1 x <2 x float>], [1 x <2 x float>]* @g
+  ret [1 x <2 x float>] %r
+}
+
+@h = global [1 x <2 x float>] [<2 x float> fadd
+      (<2 x float> bitcast (<1 x i64> <i64 10> to <2 x float>),
+       <2 x float> <float 0.0, float 10.5>)]
+
+define [1 x <2 x float>] @load2() {
+; CHECK-LABEL: _load2:
+; CHECK-NEXT:    .cfi_startproc
+; CHECK-NEXT: ; %bb.0:
+; CHECK-NEXT: Lloh2:
+; CHECK-NEXT:    adrp    x8, _h@PAGE
+; CHECK-NEXT: Lloh3:
+; CHECK-NEXT:    ldr d0, [x8, _h@PAGEOFF]
+; CHECK-NEXT:    ret
+
+  %r = load [1 x <2 x float>], [1 x <2 x float>]* @h
+  ret [1 x <2 x float>] %r
+}
+
+
+
+define [1 x <2 x float>] @promoted() {
+; CHECK-LABEL: _promoted:
+; CHECK-NEXT:     .cfi_startproc
+; CHECK-NEXT: ; %bb.0:
+; CHECK-NEXT: Lloh4:
+; CHECK-NEXT:     adrp    x8, __PromotedConst@PAGE
+; CHECK-NEXT: Lloh5:
+; CHECK-NEXT:     ldr d0, [x8, __PromotedConst@PAGEOFF]
+; CHECK-NEXT:     ret
+
+  ret [1 x <2 x float>] [<2 x float>
+      <float fneg (float extractelement (<4 x float> bitcast (<1 x i128> <i128 84405977232342107929391748327801880576> to <4 x float>), i32 0)),
+       float fneg (float extractelement (<4 x float> bitcast (<1 x i128> <i128 0> to <4 x float>), i32 1))>]
+}
+
+; CHECK-LABEL:     .section    __DATA,__data
+; CHECK-NEXT:      .globl  _g                      ; @g
+; CHECK-NEXT:      .p2align    3
+; CHECK-NEXT:  _g:
+; CHECK-NEXT:      .long   0x80000001              ; float -1.40129846E-45
+; CHECK-NEXT:      .long   0xc1280000              ; float -10.5
+
+; CHECK:           .globl  _h                      ; @h
+; CHECK-NEXT:      .p2align    3
+; CHECK-NEXT:  _h:
+; CHECK-NEXT:      .long   0x0000000a              ; float 1.40129846E-44
+; CHECK-NEXT:      .long   0x41280000              ; float 10.5
+
+; CHECK-LABEL:    .section    __TEXT,__const
+; CHECK-NEXT:     .p2align    3               ; @_PromotedConst
+; CHECK-NEXT: __PromotedConst:
+; CHECK-NEXT:     .long   0xe4c00000              ; float -2.83341989E+22
+; CHECK-NEXT:     .long   0x80000000              ; float -0

llvm/test/CodeGen/X86/complex-float-constexpr-global-initializer.ll

+; RUN: llc -o - %s | FileCheck %s
+
+target triple = "x86_64-apple-darwin"
+
+; Make sure complex floating point constant expressions are lowered properly.
+
+@g = global [1 x <2 x float>] [<2 x float>
+      <float fneg (float extractelement (<4 x float> bitcast (<1 x i128> <i128 1> to <4 x float>), i32 0)),
+       float fneg (float extractelement (<4 x float> <float 0.0, float 10.5, float 0.0, float 0.0>, i32 1))>]
+
+define [1 x <2 x float>] @load1() {
+; CHECK-LABEL:    .globl  _load1                   ## -- Begin function load
+; CHECK-NEXT:     .p2align    4, 0x90
+; CHECK-NEXT: _load1:                                  ## @load1
+; CHECK-NEXT:     .cfi_startproc
+; CHECK-NEXT: ## %bb.0:
+; CHECK-NEXT:     movsd   _g(%rip), %xmm0         ## xmm0 = mem[0],zero
+; CHECK-NEXT:     retq
+
+  %r = load [1 x <2 x float>], [1 x <2 x float>]* @g
+  ret [1 x <2 x float>] %r
+}
+
+@h = global [1 x <2 x float>] [<2 x float> fadd
+      (<2 x float> bitcast (<1 x i64> <i64 10> to <2 x float>),
+       <2 x float> <float 0.0, float 10.5>)]
+
+define [1 x <2 x float>] @load2() {
+; CHECK-LABEL:    .globl  _load2                   ## -- Begin function load2
+; CHECK-NEXT:     .p2align    4, 0x90
+; CHECK-NEXT: _load2:                                  ## @load2
+; CHECK-NEXT:     .cfi_startproc
+; CHECK-NEXT: ## %bb.0:
+; CHECK-NEXT:     movsd   _h(%rip), %xmm0         ## xmm0 = mem[0],zero
+; CHECK-NEXT:     retq
+
+  %r = load [1 x <2 x float>], [1 x <2 x float>]* @h
+  ret [1 x <2 x float>] %r
+}
+
+
+; CHECK-LABEL:     .section    __DATA,__data
+; CHECK-NEXT:     .globl  _g                      ## @g
+; CHECK-NEXT:     .p2align    3
+; CHECK-NEXT: _g:
+; CHECK-NEXT:     .long   0x80000001              ## float -1.40129846E-45
+; CHECK-NEXT:     .long   0xc1280000              ## float -10.5
+
+; CHECK:           .globl  _h                     ## @h
+; CHECK-NEXT:      .p2align    3
+; CHECK-NEXT:  _h:
+; CHECK-NEXT:      .long   0x0000000a              ## float 1.40129846E-44
+; CHECK-NEXT:      .long   0x41280000              ## float 10.5