This is an archive of the discontinued LLVM Phabricator instance.

[Evaluator] Look through invariant.group intrinsics
ClosedPublic

Authored by aeubanks on Mar 17 2021, 11:52 PM.

Details

Summary
Turning on -fstrict-vtable-pointers in Chrome caused an extra global
initializer. Turns out that a llvm.strip.invariant.group intrinsic was
causing GlobalOpt to fail to step through some simple code.

We can treat *.invariant.group uses as simply their operand.
Value::stripPointerCastsForAliasAnalysis() does exactly this. This
should be safe because the Evaluator does not skip memory accesses due
to invariants or alias analysis.

However, we don't want to leak that we've stripped arbitrary pointer
casts to users of Evaluator, so we bail out if we evaluate a function to
any constant, since we may have looked through *.invariant.group calls
and aliasing pointers cannot be arbitrarily substituted.

Diff Detail

Event Timeline

aeubanks created this revision.Mar 17 2021, 11:52 PM
aeubanks requested review of this revision.Mar 17 2021, 11:52 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 17 2021, 11:52 PM

I'm not sure if this is actually sound or not, but I think it is

aeubanks updated this revision to Diff 332470.Mar 22 2021, 4:21 PM

use Value::stripPointerCastsForAliasAnalysis()

aeubanks edited the summary of this revision. (Show Details)Mar 22 2021, 5:52 PM
rnk added a comment.Mar 23 2021, 11:58 AM

I'm trying to understand the need to special case returns bit better. I'm thinking of this example:

p = ...;
p->vcall1();
q = strip(p);
q->vcall2();

If the evaluator was used to transform this program into this:

p = ...;
p->vcall1();
p->vcall2();

... then I guess that is a bug.

I think this is only an issue if we allow interleaving the evaluator with the memory optimizations that use the invariant group metadata. If we only use Evaluator within one globalopt pass as you have in this test case, there shouldn't be any issue. I checked, and Evaluator is also used for whole program devirtualization. I can imagine a sequence of transforms involving WPD that ends up being similar to what I have above, so I guess there is a real soundness issue.

How about we allow this evaluation just for globalopt, and then we don't allow it for whole-program devirtualization?

I guess the other thing to worry about is, are we sure this really is safe for globalopt? Globalopt could, for example, evaluate a store of a pointer that has been stripped into memory, and then some later pass can load it.

Maybe a more general solution would be to restrict the operations that can be performed on a stripped pointer. Strip is really used for two things: pointer comparisons and field loads and stores. Maybe the really sound way to handle this is to tag these values in the Evaluator to forbid other kinds of operations on them. =/

llvm/lib/Transforms/Utils/Evaluator.cpp
725

I think it's "gleaned"

733

I don't think we can use the LLVM type here as a way to check if any stripped values have leaked out, since ptrtoint exists.

aeubanks added inline comments.Mar 25 2021, 4:07 PM
llvm/lib/Transforms/Utils/Evaluator.cpp
733

If you have an int derived from a group invariant pointer, it must have been stripped. If you want to cast it back to a pointer and use it, you have to launder it again anyway. The Evaluator isn't changing any existing group invariant intrinsic instructions so I believe it's safe in all cases. I believe the only issue is if we return back an SSA value from the *caller*, which the Evaluator could do.

aeubanks marked an inline comment as done.Mar 25 2021, 4:08 PM
aeubanks updated this revision to Diff 334193.Mar 30 2021, 9:26 AM

bail out when returning anything

aeubanks edited the summary of this revision. (Show Details)Mar 30 2021, 9:26 AM
rnk accepted this revision.Apr 12 2021, 2:31 PM

lgtm

My concerns are addressed now that you are checking for void types instead of pointer return types.

This revision is now accepted and ready to land.Apr 12 2021, 2:31 PM
This revision was landed with ongoing or failed builds.Apr 12 2021, 4:12 PM
This revision was automatically updated to reflect the committed changes.