Page MenuHomePhabricator

[SROA] Drop lifetime.start/end intrinsics when they block promotion.
ClosedPublic

Authored by efriedma on Sep 22 2016, 5:54 PM.

Details

Summary

Preserving lifetime markers isn't as important as allowing promotion, so just drop the lifetime markers if necessary.

This also fixes an assertion failure where other parts of SROA assumed that lifetime markers never block promotion.

Fixes https://llvm.org/bugs/show_bug.cgi?id=29139.

Diff Detail

Repository
rL LLVM

Event Timeline

efriedma updated this revision to Diff 72229.Sep 22 2016, 5:54 PM
efriedma retitled this revision from to [SROA] Expand lifetime.start/end offset checks to a couple more places..
efriedma updated this object.
efriedma added a reviewer: chandlerc.
efriedma set the repository for this revision to rL LLVM.
efriedma added subscribers: llvm-commits, Ka-Ka.
Ka-Ka added a comment.Sep 23 2016, 1:29 AM

This patch solve the problem I have in the "out of tree" backend I work with. Many thanks!

dyung added a subscriber: dyung.Sep 23 2016, 2:23 AM

I applied this patch to our codebase and it seems to fix the assertion failure we were seeing in our smaller test cases, thanks!

chandlerc edited edge metadata.Sep 30 2016, 11:46 PM

I understand that this fixes an assertion, but it does so by refusing to promote in a case where we can in fact promote.

As an alternative, have you looked at teaching the rewrite logic to "fix" (potentially be stripping) the lifetime markers during rewrite to allow the promotion cases?

efriedma updated this revision to Diff 73682.Oct 5 2016, 12:22 PM
efriedma retitled this revision from [SROA] Expand lifetime.start/end offset checks to a couple more places. to [SROA] Drop lifetime.start/end intrinsics when they block promotion..
efriedma edited edge metadata.

New approach which drops lifetime intrinsics instead of trying to preserve them.

Awesome, I'm glad this was so easy. Tiny nit picks below.

lib/Transforms/Scalar/SROA.cpp
2883–2884 ↗(On Diff #73682)

We also don't want to do this if the function is being instrumented by ASan as this will undermine scope checking using lifetime markers.

2885–2887 ↗(On Diff #73682)

Since we just always can promote through lifetime intrinsics now, I would skip the variable entirely.

efriedma updated this revision to Diff 74556.Oct 13 2016, 11:27 AM
efriedma updated this object.

Don't drop lifetime markers when ASan is enabled.

The asan checks make the patch a bit more complicated. Hopefully what I'm doing here makes sense.

chandlerc added inline comments.Oct 27 2016, 12:23 AM
lib/Transforms/Scalar/SROA.cpp
1738–1744 ↗(On Diff #74556)

This (and the bit below) seems like a really good change, and I can see why you might need to make it in order to make progress. But I think you'll need to split this out into a separate patch.

First, this needs its own test case. It can be pretty boring and just check that asan functions keep lifetime markers and avoid promoting.

But more importantly, this may have really significant knock-on effects for ASan. I just don't know. We should ask the ASan folks to test this and see what it does to things like the performance of ASan-instrumented code. My fear is that it may block *so much* promotion to registers that we actually make ASan unusably slow.

It may be necessary to instead do something much more fancy for ASan: we should instead always promote to a register but synthesize the necessary checks for ASan's use-after-scope directly without relying on memory. If this is something you're interested in playing with, I'm happy to chat with you and the ASan folks about how this might work. Otherwise, I'd suggest sending them this patch as a WIP and they can follow up when it gets to the top of their priorities.

See below for my suggestion on how to make progress in the interim though.

1954–1960 ↗(On Diff #74556)

(this is the other change I would group with the above)

2897–2901 ↗(On Diff #74556)

FWIW, I think this is sufficient to avoid regressing ASan while allowing promotion through partial lifetime markers. Is there something that breaks if you make this change and not the changes above? If so, I'd like to find a way to temporarily work around them as I think fully handling ASan + lifetime markers + promotable allocas is going to be a big project.

efriedma added inline comments.Oct 27 2016, 10:19 AM
lib/Transforms/Scalar/SROA.cpp
2897–2901 ↗(On Diff #74556)

The assertion on 2262 ("assert(CanSROA);") fails without the other changes.

I can probably narrow the other checks so they only trigger on partial lifetime markers, if you think that would be an improvement.

davide added a subscriber: davide.Nov 8 2016, 6:50 PM

I was debugging a testcase reduced from a PS4 title when I realized Eli already provided a fix
An alternative testcase, FWIW.

%myclass = type { [16 x i32] }

declare void @llvm.lifetime.start(i64, i8* nocapture)

define void @patatino() {
  %gb = alloca [2 x %myclass*]
  %tmp = bitcast [2 x %myclass*]* %gb to i8*
  call void @llvm.lifetime.start(i64 16, i8* %tmp)
  %tmp1 = getelementptr [2 x %myclass*], [2 x %myclass*]* %gb, i64 0, i64 0
  store %myclass* undef, %myclass** %tmp1
  %tmp3 = bitcast %myclass** %tmp1 to <4 x i64>*
  %tmp4 = load <4 x i64>, <4 x i64>* %tmp3
  ret void
}
chandlerc added inline comments.Nov 21 2016, 6:15 PM
lib/Transforms/Scalar/SROA.cpp
2897–2901 ↗(On Diff #74556)

OK, but making the changes above will I suspect have a much more dramatic impact on ASan.

I guess go with just always dropping partial alloca lifetime markers and leave a FIXME for the sanitizer folks to come in and re-work this.

I suspect that the sanitizer folks will need to accept the false-negatives here until they're able to move local scalar sanitizing to work in a non-shadow-memory way.

kcc added a subscriber: kcc.

+Vitaly, as this may affect asan's stack-use-after-scope detection.

In D24854#602150, @kcc wrote:

+Vitaly, as this may affect asan's stack-use-after-scope detection.

In case following the thread is confusing: the suggested approach here may cause false *negatives* for ASan + O2 + stack-use-after-scope. But anything Eli does here otherwise will regress ASan + O2 performance dramatically and fix an unknown number of current false negatives.

So I suspect the ASan folks will want to pursue changes here to reduce false negatives in a separate change where you can both avoid the optimization regressions and control enabling these checks to not suddenly expose large numbers of new failures to a check that is already deployed.

vitalybuka edited edge metadata.EditedNov 21 2016, 8:20 PM

If this patch can wait a couple of days I can investigate performance regression and amount of exposed false negatives.
It this is urgent then probably safe to do as suggested, with false-negatives, but without the risk of significant performance regression in Asan.

If this patch can wait a couple of days I can investigate performance regression and amount of exposed false negatives.
It this is urgent then probably safe to do as suggested, with false-negatives, but without the risk of significant performance regression in Asan.

I would rather go ahead. The false negatives added will be small and rare compared to those we already have IMO. And I think the risk of performance regressions and other things is really quite high. Plus, it would need a great deal more testing than we currently have, mostly around cases which ASan should catch. This patch isn't really about ASan, so it seems better to unblock it and come back and do a comprehensive job with ASan in mind later.

I would rather go ahead.

SGTM

I've rerun all out tests with the patch tonight and I see no new reports.
So even without hasFnAttribute(Attribute::SanitizeAddress) checks probability of false-negatives should be low.

If I'm following the conversation correctly, I guess this means I should go ahead and commit https://reviews.llvm.org/D24854?id=73682 (the previous version of the patch)?

chandlerc accepted this revision.Nov 22 2016, 2:26 PM
chandlerc edited edge metadata.

If I'm following the conversation correctly, I guess this means I should go ahead and commit https://reviews.llvm.org/D24854?id=73682 (the previous version of the patch)?

Yes. I would just remove the bool variable as I indicated in the comment on that revision. LGTM, go ahead and land the patch, and sorry for the back and forth, it wasn't clear to me how fast this would explode into a mess with my original ASan comment. =[

This revision is now accepted and ready to land.Nov 22 2016, 2:26 PM
This revision was automatically updated to reflect the committed changes.