Skip to content
Merged
Show file tree
Hide file tree
Changes from 1 commit
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
17 changes: 13 additions & 4 deletions clang/lib/CIR/CodeGen/CIRGenExprConstant.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -1521,10 +1521,19 @@ ConstantLValueEmitter::VisitPredefinedExpr(const PredefinedExpr *e) {

ConstantLValue
ConstantLValueEmitter::VisitAddrLabelExpr(const AddrLabelExpr *e) {
auto func = cast<cir::FuncOp>(emitter.cgf->curFn);
return cir::BlockAddrInfoAttr::get(cgm.getBuilder().getContext(),
func.getSymName(),
e->getLabel()->getName());
// A label address taken in a constant context, e.g. a static computed-goto
// dispatch table `static const void *tbl[] = {&&L1, &&L2}`. Besides emitting
// the constant, register the label as address-taken so the enclosing function
// gets an indirect-goto block with this label among its successors; otherwise
// a following `goto *tbl[i]` has no goto block to branch to. A label is
// always function-local, so cgf is set here.
assert(emitter.cgf && "label address in a constant requires a function");
CIRGenFunction &cgf = *const_cast<CIRGenFunction *>(emitter.cgf);
auto func = cast<cir::FuncOp>(cgf.curFn);
cir::BlockAddrInfoAttr info = cir::BlockAddrInfoAttr::get(
&cgf.getMLIRContext(), func.getSymName(), e->getLabel()->getName());
cgf.takeAddressOfConstantLabel(info);
return info;
}

ConstantLValue ConstantLValueEmitter::VisitCallExpr(const CallExpr *e) {
Expand Down
17 changes: 17 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenFunction.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -625,10 +625,22 @@ void CIRGenFunction::finishIndirectBranch() {
succesors.push_back(labelOp->getBlock());
rangeOperands.push_back(labelOp->getBlock()->getArguments());
}
// Labels whose address was taken only from a constant initializer have no
// function-local BlockAddressOp; add them as successors here. All labels
// are emitted by now, so the lookup resolves. A label may appear more than
// once (a dispatch table can name it twice), and each occurrence is kept as
// a distinct successor to match classic codegen.
for (cir::BlockAddrInfoAttr info : constBlockAddressLabels) {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there a reason we couldn't have used the existing blockAddressToLabel map for this?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The map is keyed by BlockAddressOp, and a label address taken in a constant initializer never produces one -- it's a #cir.block_address attribute sitting in the const array, so there's no op to key on. blockAddressInfoToLabel does key on the attr, but that one's the module-wide resolver, not the per-function set of labels this function's indirect_br needs as successors, so I kept a small per-function list instead.

Since you're landing #203644 though, I'll rebase this down to just the indirect-branch wiring on top of your BlockAddrInfoAttr and drop the duplicate attribute. That's the part that fills the errorNYI("Indirect goto without a goto block") you left in emitIndirectGotoStmt, so goto *tbl[i] through the table actually dispatches. Once it's rewritten against your attr the keying will look different anyway, so I'll match whatever fits best there.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

blockAddressToLabel is keyed by BlockAddressOp, and a label address taken in a constant context never produces one -- it's a #cir.block_addr_info constant in the initializer, with no op. So the const labels are resolved via lookupBlockAddressInfo (func+label) and tracked separately in constBlockAddressLabels, then added as cir.indirect_br successors next to the op-form labels.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think you can replace the blockAddressToLabel map with a single vector of indirect goto targets as BlockAddrInfoAttr in CGF, and you can get rid of the whole mapUnresolvedBlockAddress, mapResolvedBlockAddress, updateResolvedBlockAddress business because it's not really necessary. Then you get one container and one loop here.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Done -- there's now one indirectGotoTargets vector of BlockAddrInfoAttr, resolved in a single loop in finishIndirectBranch. The BlockAddressOp->LabelOp map and the map/unresolved/resolved helpers are gone.

cir::LabelOp labelOp = cgm.lookupBlockAddressInfo(info);
assert(labelOp && "expected cir.label to be emitted for const block addr");
succesors.push_back(labelOp->getBlock());
rangeOperands.push_back(labelOp->getBlock()->getArguments());
}
cir::IndirectBrOp::create(builder, builder.getUnknownLoc(),
indirectGotoBlock->getArgument(0), false,
rangeOperands, succesors);
cgm.blockAddressToLabel.clear();
constBlockAddressLabels.clear();
}

void CIRGenFunction::finishFunction(SourceLocation endLoc) {
Expand Down Expand Up @@ -1564,6 +1576,11 @@ void CIRGenFunction::instantiateIndirectGotoBlock() {
{builder.getUnknownLoc()});
}

void CIRGenFunction::takeAddressOfConstantLabel(cir::BlockAddrInfoAttr info) {
constBlockAddressLabels.push_back(info);
instantiateIndirectGotoBlock();

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure we should be doing this here. I see that we have code to mark the indirect goto block as poison if it has no predecessor, but why don't we just wait until we see an indirect goto statement to instantiate it?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Moved instantiation to emitIndirectGotoStmt, so a function that takes a label address but never does goto * emits no indirect branch (no more poison fallback). That diverges from classic, which still emits a dead poisoned indirectbr; h and E (label-values.c) cover it.

}

mlir::Value CIRGenFunction::emitAlignmentAssumption(
mlir::Value ptrValue, QualType ty, SourceLocation loc,
SourceLocation assumptionLoc, int64_t alignment, mlir::Value offsetValue) {
Expand Down
10 changes: 10 additions & 0 deletions clang/lib/CIR/CodeGen/CIRGenFunction.h
Original file line number Diff line number Diff line change
Expand Up @@ -730,6 +730,12 @@ class CIRGenFunction : public CIRGenTypeCache {
/// been resolved.
mlir::Block *indirectGotoBlock = nullptr;

/// Labels whose address is taken in a constant context (e.g. a static
/// computed-goto dispatch table). These have no function-local
/// BlockAddressOp, so they are tracked here and added as indirect-goto
/// branch successors in finishIndirectBranch.
llvm::SmallVector<cir::BlockAddrInfoAttr> constBlockAddressLabels;

void resolveBlockAddresses();
void finishIndirectBranch();

Expand Down Expand Up @@ -1705,6 +1711,10 @@ class CIRGenFunction : public CIRGenTypeCache {

void instantiateIndirectGotoBlock();

/// Record a label whose address is taken from a constant initializer and
/// ensure the indirect-goto block exists.
void takeAddressOfConstantLabel(cir::BlockAddrInfoAttr info);

/// Emit a simple LLVM intrinsic that takes N scalar arguments. The intrinsic
/// name is used verbatim; any overload mangling (e.g. `.f32`, `.p1`) must be
/// baked into \p intrinName by the caller. The result type defaults to the
Expand Down
127 changes: 127 additions & 0 deletions clang/test/CIR/CodeGen/goto-address-label-table.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,127 @@
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-cir %s -o %t.cir
// RUN: FileCheck --input-file=%t.cir %s --check-prefix=CIR
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -fclangir -emit-llvm %s -o %t-cir.ll
// RUN: FileCheck --input-file=%t-cir.ll %s --check-prefix=LLVM
// RUN: %clang_cc1 -triple x86_64-unknown-linux-gnu -emit-llvm %s -o %t.ll
// RUN: FileCheck --input-file=%t.ll %s --check-prefix=OGCG

int f(int x) {
static const void *tbl[] = {&&L1, &&L2};
goto *tbl[x];
L1:
return 1;
L2:
return 2;
}

// A appears twice in g's table; both occurrences are kept as indirect-branch
// successors, matching classic codegen.
int g(int x) {
static const void *tbl[] = {&&A, &&A, &&B};
goto *tbl[x];
A:
return 1;
B:
return 2;
}

// A constant label address with no indirect goto reaching it: the indirect-goto
// block is created but has no predecessors, so it is left poisoned.
int h(int x) {
static const void *tbl[] = {&&L1};
(void)tbl;
return x;
L1:
return 0;
}

// A's address comes from a constant table, B's from a runtime block-address op;
// both feed the same indirect branch.
int m(int sel) {
static const void *ctbl[] = {&&A2};
void *p = &&B2;
void *t = (void *)ctbl[0];
void *dest = sel ? t : p;
goto *dest;
A2:
return 1;
B2:
return 2;
}

// CIR-DAG: cir.global "private" internal dso_local @f.tbl = #cir.const_array<[#cir.block_addr_info<@f, "L1"> : !cir.ptr<!void>, #cir.block_addr_info<@f, "L2"> : !cir.ptr<!void>]> : !cir.array<!cir.ptr<!void> x 2>
// CIR-DAG: cir.global "private" internal dso_local @g.tbl = #cir.const_array<[#cir.block_addr_info<@g, "A"> : !cir.ptr<!void>, #cir.block_addr_info<@g, "A"> : !cir.ptr<!void>, #cir.block_addr_info<@g, "B"> : !cir.ptr<!void>]> : !cir.array<!cir.ptr<!void> x 3>
// CIR-DAG: cir.global "private" internal dso_local @h.tbl = #cir.const_array<[#cir.block_addr_info<@h, "L1"> : !cir.ptr<!void>]> : !cir.array<!cir.ptr<!void> x 1>
// CIR-DAG: cir.global "private" internal dso_local @m.ctbl = #cir.const_array<[#cir.block_addr_info<@m, "A2"> : !cir.ptr<!void>]> : !cir.array<!cir.ptr<!void> x 1>

// CIR: cir.func {{.*}} @f

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can you move these checks to below the corresponding functions and group them by function rather than having all the CIR checks, followed by all the LLVM checks, etc?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Each function's CIR and LLVM checks now sit right below it. The dispatch-table globals stay grouped at the top since they hoist to module scope in every emit.

// CIR: %[[TBL:.*]] = cir.get_global @f.tbl
// CIR: cir.indirect_br %{{.*}} : !cir.ptr<!void>, [
// CIR-NEXT: ^[[L1BB:.*]],
// CIR-NEXT: ^[[L2BB:.*]]
// CIR: ]
// CIR: ^[[L1BB]]:
// CIR: cir.label "L1"
// CIR: ^[[L2BB]]:
// CIR: cir.label "L2"

// CIR: cir.func {{.*}} @g
// CIR: cir.indirect_br %{{.*}} : !cir.ptr<!void>, [
// CIR-NEXT: ^[[ABB:.*]],
// CIR-NEXT: ^[[ABB]],
// CIR-NEXT: ^[[BBB:.*]]
// CIR: ]
// CIR: ^[[ABB]]:
// CIR: cir.label "A"
// CIR: ^[[BBB]]:
// CIR: cir.label "B"

// No indirect goto reaches the label, so the goto block is poisoned.
// CIR: cir.func {{.*}} @h
// CIR: cir.indirect_br %{{.*}} poison : !cir.ptr<!void>, [
// CIR-NEXT: ^[[HL1:.*]]
// CIR: ]
// CIR: ^[[HL1]]:
// CIR: cir.label "L1"

// A2 comes from the constant table, B2 from a runtime block-address op; both
// are successors of the one indirect branch.
// CIR: cir.func {{.*}} @m
// CIR: cir.block_address <@m, "B2">
// CIR: cir.indirect_br
// CIR-DAG: cir.label "A2"
// CIR-DAG: cir.label "B2"

// LLVM-DAG: @f.tbl = internal global [2 x ptr] [ptr blockaddress(@f, %[[L1:[0-9]+]]), ptr blockaddress(@f, %[[L2:[0-9]+]])], align 16
// LLVM-DAG: @g.tbl = internal global [3 x ptr] [ptr blockaddress(@g, %[[GA:[0-9]+]]), ptr blockaddress(@g, %[[GA]]), ptr blockaddress(@g, %[[GB:[0-9]+]])], align 16
// LLVM-DAG: @h.tbl = internal global [1 x ptr] [ptr blockaddress(@h, %{{[0-9]+}})], align 8
// LLVM-DAG: @m.ctbl = internal global [1 x ptr] [ptr blockaddress(@m, %{{[0-9]+}})], align 8

// LLVM: define dso_local i32 @f(i32 noundef %{{.*}})
// LLVM: indirectbr ptr %{{.*}}, [label %[[L1]], label %[[L2]]]

// LLVM: define dso_local i32 @g(i32 noundef %{{.*}})
// LLVM: indirectbr ptr %{{.*}}, [label %[[GA]], label %[[GA]], label %[[GB]]]

// LLVM: define dso_local i32 @h(i32 noundef %{{.*}})
// LLVM: indirectbr ptr poison, [label %{{[0-9]+}}]

// LLVM: define dso_local i32 @m(i32 noundef %{{.*}})
// LLVM: indirectbr ptr %{{.*}}, [label %{{[0-9]+}}, label %{{[0-9]+}}]

// OGCG-DAG: @f.tbl = internal global [2 x ptr] [ptr blockaddress(@f, %L1), ptr blockaddress(@f, %L2)], align 16
// OGCG-DAG: @g.tbl = internal global [3 x ptr] [ptr blockaddress(@g, %A), ptr blockaddress(@g, %A), ptr blockaddress(@g, %B)], align 16
// OGCG-DAG: @h.tbl = internal global [1 x ptr] [ptr blockaddress(@h, %L1)], align 8
// OGCG-DAG: @m.ctbl = internal global [1 x ptr] [ptr blockaddress(@m, %A2)], align 8

// OGCG: define dso_local i32 @f(i32 noundef %{{.*}})

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can the LLVM and OGCG checks be combined?

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

f/g/m share one LLVM prefix now. h can't: CIR emits no indirect branch there while classic emits the poison one, so its branch check stays split (LLVMCIR/OGCG).

// OGCG: indirectbr ptr %indirect.goto.dest, [label %L1, label %L2]

// OGCG: define dso_local i32 @g(i32 noundef %{{.*}})
// OGCG: indirectbr ptr %indirect.goto.dest, [label %A, label %A, label %B]

// OGCG: define dso_local i32 @h(i32 noundef %{{.*}})
// OGCG: indirectbr ptr poison, [label %L1]

// OGCG: define dso_local i32 @m(i32 noundef %{{.*}})
// OGCG: indirectbr ptr %indirect.goto.dest, [label %{{.*}}, label %{{.*}}]