Page MenuHomePhabricator

Strip undef implying attributes when moving calls
ClosedPublic

Authored by anna on Jun 21 2021, 6:53 AM.

Details

Summary

When hoisting/moving calls to locations, we strip unknown metadata. Such calls are usually marked speculatable, i.e. they are guaranteed to not cause undefined behaviour when run anywhere. So, we should strip attributes that can cause immediate undefined behaviour if those attributes are not valid in the context where the call is moved to.

Fix for PR50744.

Diff Detail

Event Timeline

anna created this revision.Jun 21 2021, 6:53 AM
anna requested review of this revision.Jun 21 2021, 6:53 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 21 2021, 6:53 AM
anna updated this revision to Diff 353358.Jun 21 2021, 7:03 AM

updated comments

lebedev.ri added a subscriber: lebedev.ri.EditedJun 21 2021, 7:07 AM

Should this not be integrated into Instruction::hasMetadataOtherThanDebugLoc()/Instruction::dropUnknownNonDebugMetadata() ?

anna added a comment.Jun 21 2021, 7:30 AM

Should this not be integrated into Instruction::hasMetadataOtherThanDebugLoc()/Instruction::dropUnknownNonDebugMetadata() ?

I'm not too sure frankly. There's two differences, so if we integrate it, we will need something to reflect that in the name:

  1. Attribute stripping is only for calls. On other instructions, attributes are needed for correctness (while metadata is just for performance and can be stripped without any impact on correctness).
  2. As mentioned in #1, there are attributes that are needed for correctness. So, we keep a whitelist of attributes we can legally strip from the call parameters.

On the other hand though, it makes sense to strip such attributes not just in hoisting, but also when instructions are "moved"/cloned to another location (such as stripping metadata done in SimplifyCFG). So, I can see a case for integrating this into the functionality where we drop unknown metadata.

It may be too much to overload the existing APIs, since this applies only to calls (e.g. in GVNHoist, metadata is dropped for geps, this would be unnecessary). So either doing the two calls independently as in this patch or add another API that does both.
I don't have strong opinions either way.

Is it possible this patch applies at the call in SimplifyCFG.cpp:1092 and :2487 and at hoistAllInstructionsInto in Local.cpp (also used by SimplifyCFG)? If the number of use cases is large enough, an API with both might make sense.

Side note, I believe there are cases where metadata must be dropped for correctness as well.

anna added a comment.Jun 21 2021, 8:12 PM

It may be too much to overload the existing APIs, since this applies only to calls (e.g. in GVNHoist, metadata is dropped for geps, this would be unnecessary). So either doing the two calls independently as in this patch or add another API that does both.
I don't have strong opinions either way.

Is it possible this patch applies at the call in SimplifyCFG.cpp:1092 and :2487 and at hoistAllInstructionsInto in Local.cpp (also used by SimplifyCFG)? If the number of use cases is large enough, an API with both might make sense.

It looks like both cases in SimplifyCFG and hoistAllInstructionsInto are applicable (will try constructing test cases). Also, another spot is makeLoopInvariant in Analysis/LoopInfo.cpp. There we can make a readnone speculatable call loop-invariant (and hoist it to specified point outside loop).

Side note, I believe there are cases where metadata must be dropped for correctness as well.

Ah, I meant metadata can be dropped unconditionally and that will not impact correctness. Performance can be affected if we dropped metadata that could be retained.
Whereas for attributes, if we drop them unconditionally, it can affect correctness.

I agree to your point - actually, in most cases, we drop metadata to preserve correctness.

nikic added inline comments.
llvm/lib/IR/Instructions.cpp
353 ↗(On Diff #353358)

We already have removeParamUndefImplyingAttributes() with a similar but not same list of attributes. I'm not sure if the desired semantics line up, but if they do it would be good to extend/reuse that.

jdoerfert added inline comments.Jun 23 2021, 7:52 AM
llvm/lib/IR/Instructions.cpp
350 ↗(On Diff #353358)

We got rid of "whitelist". It is also weird to call it that as we actually remove those. I'm not sure having a list here is great. Should we have a Attribute::isABIrelated() instead?

anna added inline comments.Jun 23 2021, 12:29 PM
llvm/lib/IR/Instructions.cpp
350 ↗(On Diff #353358)

Yeah. I think isABIRelated is a good choice here. I'll add that function and use it here, instead of a list. It also tests the theory that only ABI related attributes is illegal to strip.

353 ↗(On Diff #353358)

Looking at that code piece, I don't think they line up for what's required here. Specifically, it is for updating attributes if a given parameter has undefined/poison behaviour in it.

anna updated this revision to Diff 356538.Jul 5 2021, 11:15 AM

addressed review comments. Strip attributes whenever a call is moved (either through hoisting or simplifyCFG).
See updated tests.

nikic added a comment.Jul 5 2021, 12:46 PM

Do I understand correctly that a function declaration like declare void @test(i32 noundef) speculatable is illegal, because the parameter attributes can result in UB? So if the speculatable attribute is present, we can assume that all attributes present on the function declaration are also safe to speculate, and the only thing we have to worry about are additional non-speculatable attributes on the call-site?

llvm/include/llvm/IR/Instruction.h
397

nit: moves -> move

llvm/lib/IR/Attributes.cpp
279 ↗(On Diff #356538)

Shouldn't this also include at least inreg, byval, byref, preallocated, inalloca, sret? Possibly also swiftself/swiftasync and alignstack?

Might make sense to update LangRef with a clearer distinction between ABI and optimization attributes for arguments.

279 ↗(On Diff #356538)

Though generally, I'm not convinced that we should treat this as dropping all non-abi attributes.

I think we should only be dropping attributes for which passing an invalid value results in immediate undefined behavior. Dropping nonnull for example should not be needed, as that just results in a poison argument, and the speculatable attribute promises that the function must deal with that just fine.

The only attributes we need to drop are things like noundef, dereferenceable and dereferenceable_or_null. Which is pretty much what removeParamUndefImplyingAttributes() is doing (modulo implementation bugs).

llvm/lib/IR/Instruction.cpp
185

What about return attributes? Aren't they also affected?

anna added a comment.Jul 14 2021, 12:23 PM

Do I understand correctly that a function declaration like declare void @test(i32 noundef) speculatable is illegal, because the parameter attributes can result in UB?

Yes, that's right.

So if the speculatable attribute is present, we can assume that all attributes present on the function declaration are also safe to speculate, and the only thing we have to worry about are additional non-speculatable attributes on the call-site?

That's one way to reason about it. So, basically we do not need to be as restrictive as the current patch. For example, if we have the speculatable attribute on the function declaration and non null only on the parameter at the callsite, we should be able to run this call anywhere even if the parameter maybe null at the location the call is moved to. The only attributes we should strip are those that cause immediate UB. As Artur pointed out offline, it looks like nonnull behaviour was also changed recently to return poison instead of causing UB (if the pointer is indeed null). Given the speculatable attribute, we should never have UB within that function if we were to pass in a poison argument.

llvm/lib/IR/Attributes.cpp
279 ↗(On Diff #356538)

agreed. Thanks for the clarification.

llvm/lib/IR/Instruction.cpp
185

Yes, that's right. We could move the call anywhere (not just hoist it above conditions affecting the parameter attributes).

anna retitled this revision from [LICM] Strip context sensitive attributes after call hoisting to Strip undef implying attributes after call hoisting.Jul 15 2021, 11:42 AM
anna edited the summary of this revision. (Show Details)
anna retitled this revision from Strip undef implying attributes after call hoisting to Strip undef implying attributes when moving calls.
anna updated this revision to Diff 359073.Jul 15 2021, 11:43 AM

addressed review comments from @nikic.

anna marked an inline comment as done.Jul 15 2021, 11:51 AM
anna added inline comments.
llvm/lib/IR/Attributes.cpp
279 ↗(On Diff #356538)

Note that removeParamUndefImplyingAttributes does have Nonnull in it currently and the comment states that it removes attributes that imply both UB and poison.
The API was added here: https://reviews.llvm.org/D98899 and there's a comment stating that removing nonnull is strictly not necessary but doesn't hurt either. After all, we are propagating poison in that speculatable call.

My updated tests also state the same - we need not remove nonnull. I have also added tests to show where we should strictly remove attributes (such as dereferenceable).

anna updated this revision to Diff 359796.Jul 19 2021, 8:13 AM

address typo in failing testcase.

jdoerfert accepted this revision.Jul 25 2021, 9:36 AM

Rebase on 087a8eea359a should make it a bit smaller. I'm unsure why the nonnull is listed in PopulateUndefImplyingAttributes I guess after the rebase it will be gone and the tests that drop it won't anymore.

seems there are no more real concerns, so LGTM if the rebase works out as expected.

llvm/test/Transforms/LICM/call-hoisting.ll
31
This revision is now accepted and ready to land.Jul 25 2021, 9:36 AM

I was looking at the code and missed one of the patches and your comment. Does look like you agree and we can go ahead here though.

anna updated this revision to Diff 361787.Jul 26 2021, 1:55 PM

rebased over landed changes. Use getUBImplyingAttributes API.

anna marked an inline comment as done.Jul 26 2021, 1:56 PM
anna added inline comments.
llvm/lib/Analysis/IVDescriptors.cpp
202

Ah. ignore stale comment for fixing another bug.

anna added a comment.Jul 26 2021, 1:57 PM

Rebase on 087a8eea359a should make it a bit smaller. I'm unsure why the nonnull is listed in PopulateUndefImplyingAttributes I guess after the rebase it will be gone and the tests that drop it won't anymore.

seems there are no more real concerns, so LGTM if the rebase works out as expected.

Thanks @jdoerfert. I'll give it a day if anyone has concerns over the rebased code.

anna updated this revision to Diff 361790.Jul 26 2021, 2:00 PM

removed unrelated change.

lebedev.ri accepted this revision.Jul 26 2021, 2:05 PM

LGTM, thank you.
Looks like my original design comment was essentially addressed, so i really like this.

It would be best to land this within next ~12 hours or so, before branching happens,
i.e. i'd maybe suggesting landing how.

llvm/include/llvm/IR/Instruction.h
331–334

dropUnknownNonDebugMetadata()'s documentation really needs an update
to point out that dropUndefImplyingAttrsAndUnknownMetadata() likely should be used instead.

398–401

Wouldn't

void dropUndefImplyingAttrsAndUnknownMetadata(ArrayRef<unsigned> KnownIDs = {});

suffice?

nikic accepted this revision.Jul 26 2021, 2:15 PM

LG from my side as well.

anna marked 2 inline comments as done.Jul 27 2021, 7:47 AM
anna added inline comments.
llvm/include/llvm/IR/Instruction.h
398–401

agreed, done.

This revision was landed with ongoing or failed builds.Jul 27 2021, 7:57 AM
This revision was automatically updated to reflect the committed changes.
anna marked an inline comment as done.