Page MenuHomePhabricator

[deref] Use readonly to infer global dereferenceability in a callee
AbandonedPublic

Authored by reames on Mon, Mar 22, 8:19 PM.

Details

Summary

As described by code comments.

Diff Detail

Event Timeline

reames created this revision.Mon, Mar 22, 8:19 PM
reames requested review of this revision.Mon, Mar 22, 8:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptMon, Mar 22, 8:19 PM
nlopes added inline comments.Tue, Mar 23, 4:33 AM
llvm/lib/IR/Value.cpp
748

This is why I prefer atomic predicates. This if is duplicated to the code above.
If a function is readonly, doesNotFreeMemory() & hasNoSync() should return true. Otherwise we need to replicate this reasoning all over, which is going to end up with inconsistent semantics in different places.
So we are building some technical debt here..

EDIT: just checked Function.h and we have this:

bool doesNotFreeMemory() const {
   return onlyReadsMemory() || hasFnAttribute(Attribute::NoFree);
}

So I guess we only need to patch hasNoSync().

reames added inline comments.Tue, Mar 23, 9:16 AM
llvm/lib/IR/Value.cpp
748

This is a style question, and one we disagree on. I personally strongly prefer working directly with attributes and querying them directly cleaner and safer. Our predicates are often not well defined, and the attributes have at least an attempt at a definition.

If we want to infer one attribute from another, we should either a) duplicate the inference rule into a helper function and use it, or b) actually infer it and add it to the IR.

reames updated this revision to Diff 333179.Wed, Mar 24, 5:24 PM

Refresh per reviewer preference for attribute handling.

Byval, etc... split to https://reviews.llvm.org/D99310.

reames retitled this revision from [deref] Infer a few more cases of global dereferenceability in a callee to [deref] Use readonly to infer global dereferenceability in a callee.Wed, Mar 24, 5:25 PM
reames edited the summary of this revision. (Show Details)
reames planned changes to this revision.Mon, Mar 29, 5:27 PM

Realized the refactored code has a serious code smell. Not a bug per se, but not something I want to check in.

NoSync requires no coordination. ReadOnly simply implies this thread isn't initiating coordination. Another thread could be signaling, and this thread could be in a spin loop.

The only reason this isn't an active bug is that we consider all atomic ordered reads to also write.

I'm going to roll this back to the previous version.

reames updated this revision to Diff 334021.Mon, Mar 29, 5:28 PM

Roll back to previous version (undoing response to review comment) due to code smell described in last update.

I think this version should go in.

reames updated this revision to Diff 334024.Mon, Mar 29, 5:44 PM

Clarify comments to avoid (subtly misleading) reasoning about inferring nosync. While it happens to be true that atomic reads prevent readonly being inferred today, I'd rather not bake that assumption into the comment and we don't need the stronger inference anyway.

reames planned changes to this revision.Mon, Mar 29, 5:53 PM

(Thought of a counter example - of comment, not implementation)

(To flesh out later - read side check for signaled state with checkpoint mechanism solely in caller to avoid need for write while still validly syncronized.)

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

Note for anyone following along.

Given the amount this review has managed to confuse *me*, I'm planning a change in direction here. (More of a detour really.) I'm going to pause this patch and focus on having the default pipeline infer nosync in the same set of conditions that attributor does. Only once that is done will I then return and justify why readonly is a valid precondition for both inferences. I don't *think* the end result will change, but I decided I need to do a better job of making the argument with each step easy to follow and justify.

reames abandoned this revision.Thu, Apr 1, 10:14 AM

The effect of this will be entirely subsumed by D99749, and work following in that vein.