This is an archive of the discontinued LLVM Phabricator instance.

[AsmPrinter] Further restrict expressions supported in global initializers
ClosedPublic

Authored by nikic on Jun 16 2022, 7:51 AM.

Details

Summary

lowerConstant() currently accepts a number of constant expressions which have corresponding MC expressions, but which cannot be evaluated as a relocatable expression (unless the operands are constant, in which case we'll just fold the expression to a constant).

The motivation here is to clarify which constant expressions are really needed for https://discourse.llvm.org/t/rfc-remove-most-constant-expressions/63179, and in particular clarify that we do not need to support any division expressions, which are particularly problematic.

Diff Detail

Event Timeline

nikic created this revision.Jun 16 2022, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 7:51 AM
nikic requested review of this revision.Jun 16 2022, 7:51 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 16 2022, 7:51 AM
nhaehnle added inline comments.
llvm/test/CodeGen/X86/ptrtoint-constexpr.ll
12

How is this test change caused by the code change?

nikic added inline comments.Jun 21 2022, 3:36 AM
llvm/test/CodeGen/X86/ptrtoint-constexpr.ll
12

For unsupported constant expressions, we perform DataLayout-aware constant folding in attempt to remove the expression, see https://github.com/llvm/llvm-project/blob/4d2eda2bb3156cee63ea486be34b01164b178e10/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp#L2711-L2716.

nikic updated this revision to Diff 439400.Jun 23 2022, 7:59 AM

Rebase after X86 test opaquification.

I'm not a competent reviewer for this patch, but it caused me to looked up other uses of MCBinaryExpr::createDiv, I see the ARM backend is doing this:

// Otherwise it's an offset from the dispatch instruction. Construct an
// MCExpr for the entry. We want a value of the form:
// (BasicBlockAddr - TBBInstAddr + 4) / 2
//
// For example, a TBB table with entries jumping to basic blocks BB0 and BB1
// would look like:
// LJTI_0_0:
//    .byte (LBB0 - (LCPI0_0 + 4)) / 2
//    .byte (LBB1 - (LCPI0_0 + 4)) / 2
// where LCPI0_0 is a label defined just before the TBB instruction using
// this table.
MCSymbol *TBInstPC = GetCPISymbol(MI->getOperand(0).getImm());
const MCExpr *Expr = MCBinaryExpr::createAdd(
    MCSymbolRefExpr::create(TBInstPC, OutContext),
    MCConstantExpr::create(4, OutContext), OutContext);
Expr = MCBinaryExpr::createSub(MBBSymbolExpr, Expr, OutContext);
Expr = MCBinaryExpr::createDiv(Expr, MCConstantExpr::create(2, OutContext),
                               OutContext);

This is super crazy. MC should support target specific relocations like this. I'm pretty sure this has to have magic pattern matching in the assembler?

Oh no it isn't a magic relocation, it is because the assembler knows the offset between the L labels and so can constant fold it, that seems fine.

Oh no it isn't a magic relocation, it is because the assembler knows the offset between the L labels and so can constant fold it, that seems fine.

Right. For (LBB0 - (LCPI0_0 + 4)), when MCAsmLayout is available, MC knows that LHS and RHS are defined relative to the same section. The expression folds to a constant. (In RISC-V with linker relaxation, this can't).
Then, (LBB0 - (LCPI0_0 + 4)) / 2 can be folded to a constant.

MaskRay added a comment.EditedJun 28 2022, 12:28 AM

This looks good to me. switch (CE->getOpcode()) { should have a comment. The supported ConstantExpression opcodes are to support relocations for non-compile-time-constant values, so there is just a subset which is used by supported targets.

MaskRay added inline comments.Jun 28 2022, 12:30 AM
llvm/test/CodeGen/X86/nonconst-static-div.ll
2

Add a file-level comment about the purpose.

nikic updated this revision to Diff 440513.Jun 28 2022, 12:57 AM
nikic marked an inline comment as done.

Add comments

MaskRay accepted this revision.Jun 28 2022, 11:24 AM
MaskRay added inline comments.
llvm/test/CodeGen/X86/nonconst-static-div.ll
14

delete the trailing blank line.

This revision is now accepted and ready to land.Jun 28 2022, 11:24 AM
This revision was landed with ongoing or failed builds.Jun 29 2022, 1:02 AM
This revision was automatically updated to reflect the committed changes.
nikic marked an inline comment as done.