Page MenuHomePhabricator

[MemCpyOptimizer] Optimize passing byref function arguments down the stack
Needs ReviewPublic

Authored by atrosinenko on Aug 15 2020, 9:52 AM.

Details

Summary

When a byref function argument is passed through like this:

void leaf(struct X);
void middle(struct X a) {
  leaf(a);
}

... an unnecessary temporary copy may be introduced inside middle().

This patch makes that copy being eliminated by MemCpyOptimizer unless size of
the byref argument is 8 bytes or less (in that case memcpy() is eliminated by
InstCombine first preventing later optimization).

Diff Detail

Event Timeline

atrosinenko created this revision.Aug 15 2020, 9:52 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptAug 15 2020, 9:52 AM
atrosinenko requested review of this revision.Aug 15 2020, 9:52 AM

If this optimization is valid, it's valid regardless of byref.

This needs an LLVM IR test.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp
1332

I thought we have a shortcut to check for both attributes. If not, we might want one.

Restrict transformation to passing through byref arguments, not arbitrary pointers.

This looks reasonable on first glance. @arsenm ?

arsenm added inline comments.Aug 17 2020, 9:40 AM
llvm/test/Transforms/MemCpyOpt/byref-memcpy.ll
44 ↗(On Diff #286037)

I think this is missing an alignment check. Can you add a test where the callee requires a higher parameter alignment than the caller?

Sorry, I tried to say thiis more succinctly before, but what exactly is the semantic property of byref that allows this optimization?

Add a test for insufficiently aligned source.

@rjmccall

Maybe I overestimated similarity of byval and recently introduced byref... Looks like some aliasing restrictions are not mentioned in LLVM Language Reference. For example, the only way for Clang to emit the byref attribute to LLVM IR I know about is via ABIArgInfo with Kind == IndirectAliased introduced in D79744: clang: Use byref for aggregate kernel arguments - and it has a note that "The object is known to not be through any other references for the duration of the call, and the callee must not itself modify the object". This seems to be necessary for correctness of this transformation. If LLVM Language Reference does not mention such restrictions intentionally, then it may be clang that should be responsible for such optimizations.

llvm/test/Transforms/MemCpyOpt/byref-memcpy.ll
55 ↗(On Diff #286097)

Interestingly, if I change the alignment of the second argument from 2 back to 4, then memcpy() is successfully dropped (as if everything has big-enough alignment) while it seemingly shouldn't be. Is it just due to the code being ill-formed?

@rjmccall

Maybe I overestimated similarity of byval and recently introduced byref... Looks like some aliasing restrictions are not mentioned in LLVM Language Reference. For example, the only way for Clang to emit the byref attribute to LLVM IR I know about is via ABIArgInfo with Kind == IndirectAliased introduced in D79744: clang: Use byref for aggregate kernel arguments - and it has a note that "The object is known to not be through any other references for the duration of the call, and the callee must not itself modify the object". This seems to be necessary for correctness of this transformation. If LLVM Language Reference does not mention such restrictions intentionally, then it may be clang that should be responsible for such optimizations.

I think this lost the word written or stored

llvm/test/Transforms/MemCpyOpt/byref-memcpy.ll
8 ↗(On Diff #286097)

Typo attrbiute

55 ↗(On Diff #286097)

I'm not sure I follow, if you change it to 4 it is big enough?