Page MenuHomePhabricator

[WPD] Avoid noalias assumptions in unique return value optimization
ClosedPublic

Authored by inglorion on Apr 3 2020, 11:28 AM.

Details

Summary

Changes the type of the @__typeid_.*_unique_member imports we generate
for unique return value optimization from i8 to [0 x i8]. This
prevents assuming that these imports do not alias, such as when
two unique return values occur in the same vtable.

Fixes PR45393.

Diff Detail

Event Timeline

inglorion created this revision.Apr 3 2020, 11:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 3 2020, 11:28 AM
Herald added a subscriber: hiraditya. · View Herald Transcript

Naive question, but I thought LLVM IR didn't have type based alias analysis based on LLVM IR pointer types (given the intention to remove pointee types - and any cases of pointee types being significant to optimizations are bugs, so far as I know). Is that's what's going on here? Because perhaps the right fix is to fix the optimization that's making assumptions based on pointee types instead of this change?

My understanding was any type based alias analysis used the TBAA metadata to describe which pointers could/couldn't point to the same things.

@dblaikie: My understanding is that LLVM reserves the right to assume that distinct globals never alias and optimize based on that (e.g. as mentioned under https://releases.llvm.org/10.0.0/docs/AliasAnalysis.html#the-basicaa-pass), and that declaring globals as zero-sized is the accepted way to avoid that (e.g. as mentioned in https://bugs.llvm.org/show_bug.cgi?id=31675#c5).

So it's not so much improperly applied TBAA as it is an assumption that LLVM allows itself to make regardless of type, with a special exception for zero-sized types.

inglorion updated this revision to Diff 254947.Apr 3 2020, 4:44 PM

Added a comment to (hopefully) better explain what the test does.

@dblaikie: My understanding is that LLVM reserves the right to assume that distinct globals never alias and optimize based on that (e.g. as mentioned under https://releases.llvm.org/10.0.0/docs/AliasAnalysis.html#the-basicaa-pass), and that declaring globals as zero-sized is the accepted way to avoid that (e.g. as mentioned in https://bugs.llvm.org/show_bug.cgi?id=31675#c5).

So it's not so much improperly applied TBAA as it is an assumption that LLVM allows itself to make regardless of type, with a special exception for zero-sized types.

Ah - thanks! This is probably sufficiently out of my purview then, but little curious if you've time to explain it further: How does this feature work? Why could these two globals end up aliasing in practice before? Are these globals never actually defined?

rnk added a comment.Apr 6 2020, 8:25 AM

This looks good to me, given LLVM's existing behavior, but I'd like to hear from the reviewers.

I'm increasingly skeptical of the problematic optimization here, but I'm not inclined to open up discussion at the moment.

@pcc is probably the best reviewer here. I've touched WPD but only the single impl devirt part (we can't do the uniq ret value in ThinLTO index-only mode). But the changes look pretty straightforward to me, assuming this is the best solution. Looking at the associated bug, what do the defs of those symbols look like? Is one an alias of the other?

@tejohnson: The defs look like

@__typeid__ZTS4Base_16_unique_member = hidden alias i8, getelementptr (i8, i8* bitcast ({ [6 x i8*] }* @_ZTV7Derived to i8*), i64 16)
@__typeid__ZTS4Base_24_unique_member = hidden alias i8, getelementptr (i8, i8* bitcast ({ [6 x i8*] }* @_ZTV7Derived to i8*), i64 16)

So they are both aliases to the same vtable.

These are code generated into the split LTO object that is linked in in the final link step, where they show up as hidden global object, with the same value for each.

@pcc, do you have time to take a look?

pcc added inline comments.Apr 8 2020, 12:53 PM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
506

Elsewhere I've called this Int8Arr0Ty but I'm not too picky about the name.

1405

Is this change necessary? As far as I know we aren't passing the result of importGlobal here and it would be nice to avoid introducing a call to PointerType::getElementType() given that we hope to eliminate it one day.

1437

The type doesn't matter in this case (in fact it may be slightly more beneficial to use [0 x i8] here in order to allow the absolute symbols to "alias" when their values are the same), so could we just use it everywhere?

inglorion updated this revision to Diff 256196.Apr 8 2020, 11:34 PM
inglorion marked 5 inline comments as done.

Thanks, Peter! I've implemented your suggestions.

inglorion added inline comments.Apr 8 2020, 11:36 PM
llvm/lib/Transforms/IPO/WholeProgramDevirt.cpp
506

I've changed it to Int8Arr0Ty for consistency, and reworded the comment somewhat.

1405

No, we can keep this as i8. I've removed this change.

aganea added a subscriber: aganea.Apr 9 2020, 6:29 AM

@pcc, do you have time for another look?

pcc accepted this revision.Apr 16 2020, 1:45 PM

LGTM

This revision is now accepted and ready to land.Apr 16 2020, 1:45 PM
This revision was automatically updated to reflect the committed changes.