Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[CIR][CIRGen] Implement delegating constructors #821

Merged
merged 1 commit into from
Sep 9, 2024

Conversation

smeenai
Copy link
Collaborator

@smeenai smeenai commented Sep 4, 2024

This is a straightforward adaption from CodeGen. I checked the uses of
the Delegating arg that's passed in various places, and it only appears
to be used by virtual inheritance, which should be handled by
#624.

This is a straightforward adaption from CodeGen. I checked the uses of
the Delegating arg that's passed in various places, and it only appears
to be used by virtual inheritance, which should be handled by
llvm#624.
@smeenai smeenai changed the title [CIR][CodeGen] Implement delegating constructors [CIR][CIRGen] Implement delegating constructors Sep 4, 2024
};

// Check that the delegating constructor performs zero-initialization here.
// FIXME: we should either emit the trivial default constructor or remove the
Copy link
Collaborator Author

@smeenai smeenai Sep 4, 2024

Choose a reason for hiding this comment

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

To elaborate on this: CodeGen omits calls to default trivial constructors:

// If this is a call to a trivial default constructor, do nothing.
if (CD->isTrivial() && CD->isDefaultConstructor())
return;

CIRGen purposely skips that logic:

// If this is a call to a trivial default constructor:
// In LLVM: do nothing.
// In CIR: emit as a regular call, other later passes should lower the
// ctor call into trivial initialization.
// if (CD->isTrivial() && CD->isDefaultConstructor())
// return;

There doesn't actually seem to be a pass that lowers the ctor call into trivial initialization though (or at least I couldn't get such a pass to run), so the emitted LLVM IR still contains the constructor call, but the constructor is never defined, so we'll get a link error.

I tried triggering the same behavior in another way without any luck. E.g. for https://godbolt.org/z/n43n6h185, we're not emitting the default constructor call; but that's because we end up in here instead:

constant = ConstantEmitter(*this).tryEmitAbstractForInitializer(D);

The comments in that function about retaining ctor calls also seem potentially inaccurate though, or I'm misunderstanding their intent.

Copy link
Member

@bcardosolopes bcardosolopes Sep 9, 2024

Choose a reason for hiding this comment

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

There doesn't actually seem to be a pass that lowers the ctor call into trivial initialization though (or at least I couldn't get such a pass to run), so the emitted LLVM IR still contains the constructor call, but the constructor is never defined, so we'll get a link error.

That's right, we need to add support in LoweringPrepare for those. Similar / related to things tracked with should* in missing features.

The initial plan here was to add a cir.ctor_call operation, which could have attributes to designate triviality, and keep some other properties around (via AST or directly), such operation would be folded away in LoweringPrepare. It could also be emitted directly out of CIRGen or added via the idiom recognizer (I prefer the former).

None of this has been implemented yet. An alternative solution is to omit the ctor call directly (like OG codegen does) and add an assert on missing feature. Such that we can track and come back to this whenever we care about an analysis that uses this information - but in the meantime we get LLVM codegen parity.

Right now you'd have to look at a regular cir.call and try to see if it was a ctor call + determine if it's trivial, which isn't ideal since we have CIR for the very reason (raising ).

The comments in that function about retaining ctor calls also seem potentially inaccurate though, or I'm misunderstanding their intent.

The part on retaining ctor calls is legit, for purposes of static analsys we might want the allocas to be tagged properly based on ctors touching them, like the lifetime checker does. We need to keep the source semantics close first out of CIRGen.


// Check that we call the destructor whenever a cleanup is needed.
// FIXME: enable and check this when exceptions are fully supported.
#if 0
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

I wasn't sure of the best way to handle a case like this, where it'll currently NYI but we want to run the test once other support is in place. I could just not include these tests at all, but then we have to remember to add them later. By including them with an #if 0 and FIXME comments, there's at least some indication of missing coverage, though we still have to remember to enable them later.

The alternative would be creating a separate test that's currently marked XFAIL, which should start passing once the NYIs are implemented, at which point an XFAIL test passing would error out and give us a loud reminder to come back to this. The problem is that we then put the burden of fixing the test up on the person implementing the NYI, which isn't ideal either (e.g. you shouldn't have to also flesh out delegating constructor support at the same time as implementing virtual bases). Suggestions are welcome here.

Copy link
Member

Choose a reason for hiding this comment

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

I wasn't sure of the best way to handle a case like this, where it'll currently NYI but we want to run the test once other support is in place. I could just not include these tests at all, but then we have to remember to add them later. By including them with an #if 0 and FIXME comments, there's at least some indication of missing coverage, though we still have to remember to enable them later.

The alternative would be creating a separate test that's currently marked XFAIL, which should start passing once the NYIs are implemented, at which point an XFAIL test passing would error out and give us a loud reminder to come back to this. The problem is that we then put the burden of fixing the test up on the person implementing the NYI, which isn't ideal either (e.g. you shouldn't have to also flesh out delegating constructor support at the same time as implementing virtual bases). Suggestions are welcome here.

I don't think there's a silver bullet and there might a best judgment call depending on the case, some testcases also have commented code (e.g. the massive aarch64 builtins), which are very much like #if 0s. I'm fine with the #if 0 solution here, cause I know you might work on it at some point soon, so I'm not too worried. I'm usually more keen to the XFAIL idea, even though you have valid points, IMO some parts of the language / missing features are more tricky and will require some extra burden on the person implementing to get things right.

Copy link
Member

@bcardosolopes bcardosolopes left a comment

Choose a reason for hiding this comment

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

LGTM, will merge as is, but if you want to do the XFAIl approach instead, just add that in another PR.

@bcardosolopes bcardosolopes merged commit c329de7 into llvm:main Sep 9, 2024
7 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants