Page MenuHomePhabricator

[AsmPrinter] Handle foldable FP constant exprs in initializers.
AbandonedPublic

Authored by fhahn on May 12 2020, 3:38 AM.

Details

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.

Diff Detail

Event Timeline

fhahn created this revision.May 12 2020, 3:38 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 12 2020, 3:38 AM
arsenm added inline comments.May 12 2020, 6:19 AM
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 updated this revision to Diff 263429.May 12 2020, 7:13 AM

Add extract directly from vector without bitcast

fhahn marked 2 inline comments as done.May 12 2020, 7:13 AM
fhahn added inline comments.
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 added inline comments.May 12 2020, 7:38 AM
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 updated this revision to Diff 263444.May 12 2020, 8:17 AM
fhahn marked an inline comment as done.

Add constant with fadd constexpr.

fhahn marked an inline comment as done.May 12 2020, 8:17 AM
fhahn added inline comments.
llvm/test/CodeGen/AArch64/complex-float-constexpr-global-initializer.ll
10–11

Ah right. I've added a fadd constexpr.

Gerolf added a subscriber: Gerolf.May 12 2020, 12:00 PM

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?

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.

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.

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?

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 added a comment.May 13 2020, 8:31 AM

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

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.

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.

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 abandoned this revision.May 19 2020, 2:05 PM

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.