This is an archive of the discontinued LLVM Phabricator instance.

Variable auto-init of blocks capturing self after init bugfix
ClosedPublic

Authored by jfb on Feb 13 2019, 4:48 PM.

Details

Summary

Blocks that capture themselves (and escape) after initialization currently codegen wrong because this:

bool capturedByInit =
    Init && emission.IsEscapingByRef && isCapturedBy(D, Init);

Address Loc =
    capturedByInit ? emission.Addr : emission.getObjectAddress(*this);

Already adjusts Loc from thr alloca to a GEP. This code:

if (emission.IsEscapingByRef)
  Loc = emitBlockByrefAddress(Loc, &D, /*follow=*/false);

Was trying to do the same adjustment, and a GEP on a GEP (returning an int) triggers an assertion.

rdar://problem/47943027

Diff Detail

Event Timeline

jfb created this revision.Feb 13 2019, 4:48 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 13 2019, 4:48 PM
rjmccall added inline comments.Feb 13 2019, 7:22 PM
lib/CodeGen/CGDecl.cpp
1641

This seems like something we're likely regret eventually. If we've already drilled down to the storage, we should propagate that information into this rather than trying to detect it retroactively here.

jfb marked an inline comment as done.Feb 13 2019, 8:52 PM
jfb added inline comments.
lib/CodeGen/CGDecl.cpp
1641

You mean that I should check !capturedByInit instead? I initially did that, but I was worried about someone adding other drill-downs and then going out of sync. My rationale was: you always start with alloca, so anything else means a drill-down occurred.

That being said, this isn't a strongly held belief. Happy to go your way if you think it's better, you know this code more than I do :)

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

jfb updated this revision to Diff 186853.Feb 14 2019, 8:59 AM
  • Update with John's suggestion.
jfb added a comment.Feb 14 2019, 8:59 AM

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

Sounds good. I made it const too, to avoid inadvertent modification.

In D58218#1398124, @jfb wrote:

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

Sounds good. I made it const too, to avoid inadvertent modification.

This is actually the reverse of the sense I would expect it to have based on the name. If you want these semantics, maybe locIsObject?

jfb updated this revision to Diff 186950.Feb 14 2019, 5:05 PM
  • Fix polarity
jfb added a comment.Feb 14 2019, 5:05 PM
In D58218#1398124, @jfb wrote:

Well, you can always make a variable with a more directly-applicable name than capturedByInit and update it as appropriate, like locIsByrefHeader.

Sounds good. I made it const too, to avoid inadvertent modification.

This is actually the reverse of the sense I would expect it to have based on the name. If you want these semantics, maybe locIsObject?

You're right, I've fixed the polarity.

rjmccall accepted this revision.Feb 14 2019, 6:40 PM

LGTM.

This revision is now accepted and ready to land.Feb 14 2019, 6:40 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptFeb 15 2019, 9:26 AM