Page MenuHomePhabricator

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

Authored by jonpa on Wed, Jun 9, 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

Unit TestsFailed

TimeTest
30 msx64 debian > MLIR.IR::diagnostic-handler-filter.mlir
Script: -- : 'RUN: at line 1'; /var/lib/buildkite-agent/builds/llvm-project/build/bin/mlir-opt /var/lib/buildkite-agent/builds/llvm-project/mlir/test/IR/diagnostic-handler-filter.mlir -test-diagnostic-filter='filters=mysource1' -o - 2>&1 | /var/lib/buildkite-agent/builds/llvm-project/build/bin/FileCheck /var/lib/buildkite-agent/builds/llvm-project/mlir/test/IR/diagnostic-handler-filter.mlir

Event Timeline

jonpa created this revision.Wed, Jun 9, 2:50 PM
jonpa requested review of this revision.Wed, Jun 9, 2:50 PM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Jun 9, 2:50 PM
aeubanks accepted this revision.Wed, Jun 9, 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.Wed, Jun 9, 2:54 PM
rnk accepted this revision.Wed, Jun 9, 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.Wed, Jun 9, 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.Wed, Jun 9, 5:44 PM
rnk added a comment.Fri, Jun 11, 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.Tue, Jun 15, 1:39 PM
jonpa updated this revision to Diff 352286.EditedTue, Jun 15, 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.Wed, Jun 16, 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.Thu, Jun 17, 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

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.EditedSun, Jun 20, 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

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?