This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine / BuildLibCalls] Add parameter attributes from the prototype.
ClosedPublic

Authored by jonpa on Jun 9 2021, 2:50 PM.

Details

Summary

Currently only the calling convention is passed on from the prototype to the built call instruction. This patch also copies the parameter attributes.

This seems to be needed after D101806, which changed the instruction selector to only look at the call instruction parameter attributes, whereas before it also looked at the called function.

There are probably more places that needs a similar fix, like builtins..?

This makes the "verifier extension" (D103412) pass on SPEC17/SystemZ. Not sure if the returned value is checked by that patch or if they need a similar handling like the parameters?

Diff Detail

Event Timeline

jonpa created this revision.Jun 9 2021, 2:50 PM
jonpa requested review of this revision.Jun 9 2021, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptJun 9 2021, 2:50 PM
aeubanks accepted this revision.Jun 9 2021, 2:54 PM

I reverted that change and probably won't revisit it soon due to all these sorts of issues.
But this change looks good on its own

llvm/test/Transforms/InstCombine/libcall-param-attrs.ll
3

is this necessary?

This revision is now accepted and ready to land.Jun 9 2021, 2:54 PM
rnk accepted this revision.Jun 9 2021, 2:56 PM

+1

llvm/test/Transforms/InstCombine/libcall-param-attrs.ll
3

You should be able to pass -mtriple to make this test work.

jonpa updated this revision to Diff 351014.Jun 9 2021, 5:43 PM

Thanks for review!

This was not quite complete to commit as it was a suggestion and there were still the tests updates to do...

I could update most of the tests with update_test_checks.py, and most of the remaining manually (not quite sure if the function attribute '#0' tag should be part of the test line or not, but I think it shouldn't matter).

However, there are now two remaining tests that the verifier rejects, but I am not quite sure how to fix them:

LLVM :: Transforms/InstCombine/strcpy_chk-1.ll
LLVM :: Transforms/InstCombine/strncpy_c

Instcombiner builds the memcpy, but this time the 'returned' attribute is included as a parameter attribute. That makes Verifier.cpp (line 1962) do the check (which it simply omitted before)

Assert(Ty->canLosslesslyBitCastTo(FT->getReturnType())

where Ty is '*i8', and FT->getReturnType() is 'void'.

There doesn't really seem to be an error as the call looks like

call void @llvm.memcpy.p0i8.p0i8.i32(i8* noalias noundef nonnull returned ...

I suppose getReturnType() should in this case return '*i8' and not void. Or possibly the Verifier could be adapted to understand this check?

Given that the problematic patch was reverted, I wonder now if this is still relevant and needed and we should fix the above problem now?

jonpa requested review of this revision.Jun 9 2021, 5:44 PM
rnk added a comment.Jun 11 2021, 4:45 PM

I think the verifier is doing the right thing, I think it's a bug that @llvm.memcpy is carrying a returned attribute on the first argument. I'm not sure how that is happening, but whatever sets that up seems like a bug.

jonpa added a subscriber: efriedma.

I think the verifier is doing the right thing, I think it's a bug that @llvm.memcpy is carrying a returned attribute on the first argument. I'm not sure how that is happening, but whatever sets that up seems like a bug.

That was added by 8715e03 / https://reviews.llvm.org/D51092. I tried reverting that and got some test failures, whereof one in DeadStoreElimination, so it seems these are useful in some way...

@efriedma: You have any comment on how to proceed here (given you reviewed that revision)?

D51092 is marking the libc memcpy function, which is correct. Not sure how the attribute is getting carried over to llvm.memcpy.

ormris removed a subscriber: ormris.Jun 15 2021, 1:39 PM
jonpa updated this revision to Diff 352286.EditedJun 15 2021, 4:21 PM

D51092 is marking the libc memcpy function, which is correct. Not sure how the attribute is getting carried over to llvm.memcpy.

Ah, I see. This is what happened:

call i8* @__strcpy_chk

=> FortifiedLibCallSimplifier::optimizeStrpCpyChk =>

call i8* @strcpy(i8* noalias returned writeonly getelementptr inbounds...

=> optimizeStringMemoryLibCall() => optimizeStrCpy() => CreateMemCpy => 

*NewCI = CreateMemTransferInst(Intrinsic::memcpy);
NewCI->setAttributes(CI->getAttributes());

So the llvm.memcpy ends up taking the attributes from the created strcpy...

To remedy this, I made a new helper function takeAttributesFrom() that makes sure that attributes transferred onto llvm.memcpy has the Returned attribute removed if present. With that, the tests are passing.

Does this seem right, and if so, I suppose this should be done with the other intrinsics as well, e.g. after CreateMemSet()?

Taking all the attributes from a call to one function and applying them to a call to a different function doesn't make sense to me. Different functions have different attributes.

There might be a few specific attributes that make sense to transfer, but you'd want to explicitly handle the specific attributes that are relevant. (For example, pointer alignment.)

jonpa added a comment.Jun 16 2021, 8:32 AM

Taking all the attributes from a call to one function and applying them to a call to a different function doesn't make sense to me. Different functions have different attributes.

There might be a few specific attributes that make sense to transfer, but you'd want to explicitly handle the specific attributes that are relevant. (For example, pointer alignment.)

It seems that all callers of CreateMemCpy() so far has assumed that taking the attributes of the original call is the right thing to do. The only change here is to not take the Returned attribute.

If this is incorrect, I suppose I should not create the helper function and just remove the Returned attribute where needed for now?

Can we just completely skip transferring the attributes to the memcpy?

rnk added a comment.Jun 17 2021, 12:46 PM

I agree, we probably need to better understand what attributes we are trying to transfer. My guess is that we are trying to transfer call site attributes, things like noinline, alwaysinline, or other things inferred by optimization. If the call's prototype changes, then copying argument attributes doesn't make any sense and we should re-evaluate doing it in the first place.

llvm/lib/Transforms/Utils/BuildLibCalls.cpp
1222

This change is fine: we are taking the attributes from a known callee.

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
199 ↗(On Diff #352286)

This seems questionable. Why not take the parameter attributes from NewCI's callee? What is the value of merging in the attributes from the old call?

jonpa updated this revision to Diff 353238.EditedJun 20 2021, 12:24 PM
jonpa marked 2 inline comments as done.

Can we just completely skip transferring the attributes to the memcpy?

I removed takeAttributesFrom() and this seemed to not make much of a difference at all, at least not on SPEC17/SystemZ (NFC). Some InstCombine tests changed though - not sure if that look ok or not...

There are also other calls, such as CreateMemSet()/CreateMemMove() that still take all the attributes in the same way from the original CallInst... Maybe these should be removed now as well, or are they different?

I see in LibCallSimplifier::optimizeStrNCpy() a transfer of the parameter attributes only, which may make sense though...

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
199 ↗(On Diff #352286)

Not sure if there could be any value of taking parameter attributes from the old call... It seems though that the call to CreateMemCpy already passes on explicit attributes (alignment), so I suppose that might be enough to do in that place?

Is there anything similar to inferLibFuncAttributes() for these intrinsics that we should use?

rnk added a subscriber: xbolva00.Jun 23 2021, 9:15 PM
rnk added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1120–1121 ↗(On Diff #353238)

This change makes this comment stale. I checked using blame, and this setAttributes call was added in rGe80fcf03407acd6429d07e4a45185ac546ffa37c by @xbolva00 to propagate the nonnull annotations. However, I don't see any changes to the tests that were added, so maybe we don't need this any longer. Maybe the attributes are present on the memcpy declaration already.

In conclusion, I think the change looks good, just remove this stale comment. @xbolva00 , feel free to check that logic.

jonpa updated this revision to Diff 354316.Jun 24 2021, 11:27 AM

Stale comment removed

jonpa marked an inline comment as done.Jun 24 2021, 11:28 AM
jonpa added inline comments.
llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1120–1121 ↗(On Diff #353238)

Removed (not sure about leaving the 'TODO'..?)

rnk accepted this revision.Jun 24 2021, 11:41 AM

lgtm, go ahead and remove the TODO before landing

llvm/lib/Transforms/Utils/SimplifyLibCalls.cpp
1120–1121 ↗(On Diff #353238)

The TODO was added in D95088, which you have removed the need for. I believe it can also be removed.

This revision is now accepted and ready to land.Jun 24 2021, 11:41 AM
This revision was automatically updated to reflect the committed changes.
jonpa marked an inline comment as done.

This is somehow adding the returned attribute to a memcpy intrinsic on mac: https://crbug.com/1223647.
I'll dig deeper tomorrow into what's going wrong