[CIR][CIRGen] Support a defined pure virtual destructor #825
Conversation
| @@ -1109,7 +1109,17 @@ void CIRGenFunction::buildDestructorBody(FunctionArgList &Args) { | |||
| // in fact emit references to them from other compilations, so emit them | |||
| // as functions containing a trap instruction. | |||
| if (DtorType != Dtor_Base && Dtor->getParent()->isAbstract()) { | |||
| llvm_unreachable("NYI"); | |||
| builder.create<mlir::cir::TrapOp>(getLoc(Dtor->getLocation())); | |||
There was a problem hiding this comment.
I'm not sure if this is the best location to use, but I'm also not sure if it matters very much.
There was a problem hiding this comment.
If Dtor->getBody() is available, getting the loc of that stmt would be the best effort.
| // cir.trap is a terminator. The corresponding IRGen code does this: | ||
| // builder.clearInsertionPoint(); | ||
| // However, CIRGenFunction::generateCode performs the following check, which | ||
| // fails if we do the same thing here: | ||
| // assert(builder.getInsertionBlock() && "Should be valid"); | ||
| // IRGen has no equivalent assert, but for now we create a block to have a | ||
| // valid insertion point. | ||
| // TODO(cir): Should the CIRGenFunction::generateCode assert be relaxed? |
There was a problem hiding this comment.
I'd appreciate advice on this.
There was a problem hiding this comment.
MLIR builder asserts the insertion point to be valid when inserting operations while LLVMIR builder does not. Thus CodeGen could clear the insertion point after emitting an terminator but CIRGen could not.
The assert statement in CIRGenFunction::generateCode ensures that the cleanup of the function-level lexical scope could be successfully generated in LexicalScope::~LexicalScope. Note that the cleanup also includes the return operation of the function. Since we currently have no means to track whether all paths to the end of a function is non-returning, the return op has to be emitted, thus the insertion point has to be kept alive by now.
The workaround is to start a new block that receives any ops generated after emitting a terminator op in the function body. This new block will be unreachable and will be elided by follow-up passes.
There was a problem hiding this comment.
The fact that OG codegen (IRGen) clears insertion points is sometimes a bit brittle (used to signal some behavior that could be tracked more clearly). When emitting initializer for globals in CIR we sometimes also have no insertion point because those don't come from a regular function, and that's fine over there.
Just remove the overall question in form of comment and add a note saying that OG checks the insertion point but we do not.
| // cir.trap is a terminator. The corresponding IRGen code does this: | ||
| // builder.clearInsertionPoint(); | ||
| // However, CIRGenFunction::generateCode performs the following check, which | ||
| // fails if we do the same thing here: | ||
| // assert(builder.getInsertionBlock() && "Should be valid"); | ||
| // IRGen has no equivalent assert, but for now we create a block to have a | ||
| // valid insertion point. | ||
| // TODO(cir): Should the CIRGenFunction::generateCode assert be relaxed? |
There was a problem hiding this comment.
The fact that OG codegen (IRGen) clears insertion points is sometimes a bit brittle (used to signal some behavior that could be tracked more clearly). When emitting initializer for globals in CIR we sometimes also have no insertion point because those don't come from a regular function, and that's fine over there.
Just remove the overall question in form of comment and add a note saying that OG checks the insertion point but we do not.
This is permitted by the language, and IRGen emits traps for destructors other than the base object destructor. Make CIRGen follow suit.
smeenai
force-pushed
the
defined-pure-virtual
branch
from
1c6c582 to
c25189a
Compare
| @@ -1109,7 +1109,14 @@ void CIRGenFunction::buildDestructorBody(FunctionArgList &Args) { | |||
| // in fact emit references to them from other compilations, so emit them | |||
| // as functions containing a trap instruction. | |||
| if (DtorType != Dtor_Base && Dtor->getParent()->isAbstract()) { | |||
| llvm_unreachable("NYI"); | |||
| SourceLocation Loc = | |||
| Dtor->hasBody() ? Dtor->getBody()->getBeginLoc() : Dtor->getLocation(); | |||
There was a problem hiding this comment.
It'd make slightly more sense if the trap is has the getEndLoc instead.
|
|
||
| // The base object destructor should be emitted as normal. | ||
| // CHECK-LABEL: cir.func @_ZN1CD2Ev(%arg0: !cir.ptr<!ty_C> loc({{[^)]+}})) {{.*}} { | ||
| // CHECK-NEXT: %0 = cir.alloca !cir.ptr<!ty_C>, !cir.ptr<!cir.ptr<!ty_C>>, ["this", init] {alignment = 8 : i64} |
There was a problem hiding this comment.
In general, the tests should use regex for the SSA values, should make them a little more resistant to overall CIR changes.
|
Ack on the feedback, will address it when I can. |
This is permitted by the language, and IRGen emits traps for destructors other than the base object destructor. Make CIRGen follow suit.
This is permitted by the language, and IRGen emits traps for destructors other than the base object destructor. Make CIRGen follow suit.
This is permitted by the language, and IRGen emits traps for destructors other than the base object destructor. Make CIRGen follow suit.
This is permitted by the language, and IRGen emits traps for destructors other than the base object destructor. Make CIRGen follow suit.
This is permitted by the language, and IRGen emits traps for destructors other than the base object destructor. Make CIRGen follow suit.
This is permitted by the language, and IRGen emits traps for destructors
other than the base object destructor. Make CIRGen follow suit.