Page MenuHomePhabricator

[deref-at-point] restrict inference of dereferenceability based on allocsize attribute
ClosedPublic

Authored by reames on Feb 1 2021, 12:58 PM.

Details

Summary

(Patch description completely redone.)

Support deriving dereferenceability facts from allocation sites with known object sizes while correctly accounting for any possibly frees between allocation and use site. (At the moment, that's super conservative and only allowing it in functions where we know we can't free.)

This is part of the work on deref-at-point semantics. I'm making the change unconditional as the miscompile in this case is way too easy to trip by accident, and the optimization was only recently added (by me).

After this lands, the TLI wiring will be done separately in another review. That way a single review isn't both fixing a bug, and enabling further optimization.

Diff Detail

Event Timeline

reames created this revision.Feb 1 2021, 12:58 PM
reames requested review of this revision.Feb 1 2021, 12:58 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 1 2021, 12:58 PM

FWIW, the conceptual problem was known and D61652 was supposed to address this. We never merged it and we might not need it since we have free now.

reames updated this revision to Diff 322524.Feb 9 2021, 3:53 PM
reames retitled this revision from [WIP] Demonstrate correctness problem with deref attributes and free to Restrict hoisting of potentially freed memory .
reames edited the summary of this revision. (Show Details)
reames added a reviewer: jdoerfert.
reames set the repository for this revision to rG LLVM Github Monorepo.

Rework of patch description, and candidate fix for issue exposed in first WIP.

reames updated this revision to Diff 322529.Feb 9 2021, 4:05 PM
reames edited the summary of this revision. (Show Details)

Really should have made sure the last build had fully finished running make check before posting patch...

jdoerfert added inline comments.Feb 10 2021, 9:31 PM
llvm/lib/Analysis/MemoryBuiltins.cpp
526 ↗(On Diff #322529)

I think we need to address some of the TODOs to avoid regressions.

reames added inline comments.Feb 11 2021, 10:32 AM
llvm/lib/Analysis/MemoryBuiltins.cpp
526 ↗(On Diff #322529)

As stated in the review description, the regression is intentional. As the author of the change being partially reverted, I think this is the right step forward.

reames updated this revision to Diff 333187.Wed, Mar 24, 6:17 PM
reames retitled this revision from Restrict hoisting of potentially freed memory to (Corrrectly) support hoisting of loads following calls to noalias functions.
reames edited the summary of this revision. (Show Details)

Rebase over the infrastructure added for deref-at-point semantics. This is effectively unconditionally using deref-at-point, but since if we don't, we have a miscompile actively happening, I'm not too worried about that.

reames abandoned this revision.Wed, Mar 24, 6:44 PM

While trying to add the test case, ran across something weird. Not sure if the patch is wrong or my brain is just fried. Will take a fresh look tomorrow.

reames reclaimed this revision.Wed, Mar 24, 6:44 PM
reames planned changes to this revision.
reames updated this revision to Diff 333425.Thu, Mar 25, 2:31 PM
reames retitled this revision from (Corrrectly) support hoisting of loads following calls to noalias functions to [deref-at-point] restrict inference of dereferenceability based on allocsize attribute.
reames edited the summary of this revision. (Show Details)
reames added a reviewer: nikic.

Restrict scope further - only fix the existing latent issue w/allocsize.

(Good news, the weirdness from last night was human error.)

reames added inline comments.Thu, Mar 25, 2:42 PM
llvm/test/Transforms/LoopVectorize/X86/load-deref-pred.ll
95

Please ignore these changes. This a result of autogen format change. I have landed a change which includes these, and the next rebase will wipe them out. I'd usually do that now, but build times are substantial given the Value.h change. :)

jdoerfert accepted this revision.Wed, Mar 31, 9:29 PM

LG, thanks for cleaning this up.

This revision is now accepted and ready to land.Wed, Mar 31, 9:29 PM
nikic added a comment.Thu, Apr 1, 12:55 AM

As we don't have any default-enabled nosync inference, doesn't this effectively disable this code entirely (for the default pipeline)?

reames added a comment.Thu, Apr 1, 8:00 AM

As we don't have any default-enabled nosync inference, doesn't this effectively disable this code entirely (for the default pipeline)?

Not really.

Remember, the currently hooked up code doesn't get the benefit of TLI. (Because doing so immediately broke badly.) As a result, it only handles things which are a) allocas and the like, or b) explicitly marked allocsize. We do take a hit on the allocsize cases - except that the single consumer I know of is a GCd frontend which is special cased in the canBeFreed stuff anyways. If anything, this should give us a net benefit because we can expose the malloc/new cases by adding TLI as a parameter (to be done and reviewed separately.)

This revision was landed with ongoing or failed builds.Thu, Apr 1, 8:35 AM
This revision was automatically updated to reflect the committed changes.