[AsmPrinter] Further restrict expressions supported in global initializers

[nikic]

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.

[nhaehnle]

llvm/test/CodeGen/X86/ptrtoint-constexpr.ll

12

How is this test change caused by the code change?

[nikic]

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]

Rebase after X86 test opaquification.

[lattner]

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?

[lattner]

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.

[MaskRay]

In D127972#3613450, @lattner wrote:

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]

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]

llvm/test/CodeGen/X86/nonconst-static-div.ll

2

Add a file-level comment about the purpose.

[nikic]

Add comments

[MaskRay]

llvm/test/CodeGen/X86/nonconst-static-div.ll

14

delete the trailing blank line.

llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp

const MCExpr *AsmPrinter::lowerConstant(const Constant *CV) {
   if (const NoCFIValue *NC = dyn_cast<NoCFIValue>(CV))
     return MCSymbolRefExpr::create(getSymbol(NC->getGlobalValue()), Ctx);
 
   const ConstantExpr *CE = dyn_cast<ConstantExpr>(CV);
   if (!CE) {
     llvm_unreachable("Unknown constant value to lower!");
   }
 
+  // The constant expression opcodes are limited to those that are necessary
+  // to represent relocations on supported targets. Expressions involving only
+  // constant addresses are constant folded instead.
   switch (CE->getOpcode()) {
   case Instruction::AddrSpaceCast: {
     const Constant *Op = CE->getOperand(0);
     unsigned DstAS = CE->getType()->getPointerAddressSpace();
     unsigned SrcAS = Op->getType()->getPointerAddressSpace();
     if (TM.isNoopAddrSpaceCast(SrcAS, DstAS))
       return lowerConstant(Op);
 
if (IsConstantOffsetFromGlobal(CE->getOperand(0), LHSGV, LHSOffset,
         }
         int64_t Addend = (LHSOffset - RHSOffset).getSExtValue();
         if (Addend != 0)
           RelocExpr = MCBinaryExpr::createAdd(
               RelocExpr, MCConstantExpr::create(Addend, Ctx), Ctx);
         return RelocExpr;
       }
     }
-  }
-  // else fallthrough
-  LLVM_FALLTHROUGH;
 
-  // The MC library also has a right-shift operator, but it isn't consistently
-  // signed or unsigned between different targets.
-  case Instruction::Add:
-  case Instruction::Mul:
-  case Instruction::SDiv:
-  case Instruction::SRem:
-  case Instruction::Shl:
-  case Instruction::And:
-  case Instruction::Or:
-  case Instruction::Xor: {
     const MCExpr *LHS = lowerConstant(CE->getOperand(0));
     const MCExpr *RHS = lowerConstant(CE->getOperand(1));
-    switch (CE->getOpcode()) {
-    default: llvm_unreachable("Unknown binary operator constant cast expr");
-    case Instruction::Add: return MCBinaryExpr::createAdd(LHS, RHS, Ctx);
-    case Instruction::Sub: return MCBinaryExpr::createSub(LHS, RHS, Ctx);
-    case Instruction::Mul: return MCBinaryExpr::createMul(LHS, RHS, Ctx);
-    case Instruction::SDiv: return MCBinaryExpr::createDiv(LHS, RHS, Ctx);
-    case Instruction::SRem: return MCBinaryExpr::createMod(LHS, RHS, Ctx);
-    case Instruction::Shl: return MCBinaryExpr::createShl(LHS, RHS, Ctx);
-    case Instruction::And: return MCBinaryExpr::createAnd(LHS, RHS, Ctx);
-    case Instruction::Or:  return MCBinaryExpr::createOr (LHS, RHS, Ctx);
-    case Instruction::Xor: return MCBinaryExpr::createXor(LHS, RHS, Ctx);
-    }
+    return MCBinaryExpr::createSub(LHS, RHS, Ctx);
+    break;
+
+  case Instruction::Add: {
+    const MCExpr *LHS = lowerConstant(CE->getOperand(0));
+    const MCExpr *RHS = lowerConstant(CE->getOperand(1));
+    return MCBinaryExpr::createAdd(LHS, RHS, Ctx);
   }
   }
 }
 
 static void emitGlobalConstantImpl(const DataLayout &DL, const Constant *C,
                                    AsmPrinter &AP,
                                    const Constant *BaseCV = nullptr,
                                    uint64_t Offset = 0);

llvm/test/CodeGen/X86/nonconst-static-div.ll

+; RUN: not --crash llc -mtriple=i686-linux-gnu < %s 2>&1 | FileCheck %s
+
+; Targets only support a limited set of relocations. Make sure that unusual
+; constant expressions (and in particular potentially trapping ones involving
+; division) are already rejected when lowering the Constant to the MC layer,
+; rather than only when trying to emit a relocation. This makes sure that an
+; error is reported when targeting assembly (without -filetype=obj).
+
+@g = external global i32
+@g2 = global i64 sdiv (i64 3, i64 ptrtoint (ptr @g to i64))
+
+; CHECK: Unsupported expression in static initializer: sdiv

llvm/test/CodeGen/X86/ptrtoint-constexpr.ll

 ; RUN: llc < %s -mtriple=i386-linux | FileCheck %s
   %union.x = type { i64 }
 
 ; CHECK:  .globl r
 ; CHECK: r:
 ; CHECK: .quad  r&4294967295
 
 @r = global %union.x { i64 ptrtoint (ptr @r to i64) }, align 4
 
 ; CHECK:  .globl x
 ; CHECK: x:
-; CHECK: .quad  ((0+1)&4294967295)*3
+; CHECK: .quad  3
 
 @x = global i64 mul (i64 3, i64 ptrtoint (ptr getelementptr (i2, ptr null, i64 1) to i64))