This is an archive of the discontinued LLVM Phabricator instance.

Differential D151626

[MemCpyOpt] Remove byval to clarify test case for smaller sized memcpy(NFC)
ClosedPublic

Authored by khei4 on May 28 2023, 12:52 AM.

Details

Reviewers
nikic
Commits
rG0ad88c156ebd: [MemCpyOpt] remove byval from memcpy size crtical test(NFC)
Summary

While reading MemCpyOpt's processByValArgument, removing alignment check here causes failure in a test named smaller.ll
attributing align to its argument reduces this case, so remove byval attribute on call site.

I think smaller size memcpy than the original type is valid and should be reduced in MemCpyOpt but in this case, I think the problem is unknown alignment for the arg.

Diff Detail

Event Timeline

khei4 created this revision.May 28 2023, 12:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2023, 12:52 AM
Herald added a subscriber: StephenFan. · View Herald Transcript
khei4 requested review of this revision.May 28 2023, 12:52 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 28 2023, 12:52 AM
Herald added a subscriber: llvm-commits. · View Herald Transcript
Harbormaster completed remote builds in B235056: Diff 526312.May 28 2023, 1:41 AM
nikic added a comment.May 28 2023, 3:10 AM

Based on the commit that introduced the test (https://github.com/llvm/llvm-project/commit/19e30d5a7d3369feae36cbe7f4f4c1f358881c41), this is indeed about the length arguments, but for a different transform. It's not intended to test the byval transform at all. I think it would be best to drop the byval attribute from this test.

khei4 planned changes to this revision.May 28 2023, 11:44 PM

@nikic Ah, thanks! I failed to find its commit.

Although not sure its size criteria are necessary(I'll see), this patch removes byval for clarification, thanks!

khei4 updated this revision to Diff 526398.May 28 2023, 11:47 PM
khei4 edited the summary of this revision. (Show Details)

remove byval

khei4 retitled this revision from [MemCpyOpt] clarify the comment in byval alignment test(NFC) to [MemCpyOpt] clarify test case for smaller sized memcpy(NFC).May 28 2023, 11:51 PM
khei4 retitled this revision from [MemCpyOpt] clarify test case for smaller sized memcpy(NFC) to [MemCpyOpt] remove byval to clarify test case for smaller sized memcpy(NFC).
khei4 retitled this revision from [MemCpyOpt] remove byval to clarify test case for smaller sized memcpy(NFC) to [MemCpyOpt] Remove byval to clarify test case for smaller sized memcpy(NFC).
nikic accepted this revision.May 29 2023, 12:09 AM

LGTM

This revision is now accepted and ready to land.May 29 2023, 12:09 AM
Harbormaster completed remote builds in B235123: Diff 526398.May 29 2023, 12:22 AM
khei4 updated this revision to Diff 526403.May 29 2023, 12:25 AM

rebase

This revision was landed with ongoing or failed builds.May 29 2023, 12:56 AM
Closed by commit rG0ad88c156ebd: [MemCpyOpt] remove byval from memcpy size crtical test(NFC) (authored by khei4). · Explain Why
This revision was automatically updated to reflect the committed changes.
khei4 added a commit: rG0ad88c156ebd: [MemCpyOpt] remove byval from memcpy size crtical test(NFC).
Harbormaster completed remote builds in B235127: Diff 526403.May 29 2023, 12:58 AM

Revision Contents

PathSize
llvm/
test/
Transforms/
MemCpyOpt/
6 lines

Diff 526405

llvm/test/Transforms/MemCpyOpt/smaller.ll

Show All 13 Lines
declare void @check(ptr byval(%struct.s) %p) nounwind declare void @check(ptr byval(%struct.s) %p) nounwind
declare void @llvm.memcpy.p0.p0.i32(ptr nocapture, ptr nocapture, i32, i1) nounwind declare void @llvm.memcpy.p0.p0.i32(ptr nocapture, ptr nocapture, i32, i1) nounwind
define void @foo() nounwind { define void @foo() nounwind {
; CHECK-LABEL: @foo( ; CHECK-LABEL: @foo(
; CHECK-NEXT: entry: ; CHECK-NEXT: entry:
; CHECK-NEXT: [[AGG_TMP:%.*]] = alloca [[STRUCT_S:%.*]], align 4 ; CHECK-NEXT: [[AGG_TMP:%.*]] = alloca [[STRUCT_S:%.*]], align 4
; CHECK-NEXT: store i32 99, ptr getelementptr inbounds (%struct.s, ptr @cell, i32 0, i32 1), align 4 ; CHECK-NEXT: store i32 99, ptr getelementptr inbounds ([[STRUCT_S]], ptr @cell, i32 0, i32 1), align 4
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr align 1 @cell, ptr align 1 @.str, i32 11, i1 false) ; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr align 1 @cell, ptr align 1 @.str, i32 11, i1 false)
; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr align 4 [[AGG_TMP]], ptr align 4 @cell, i32 16, i1 false) ; CHECK-NEXT: call void @llvm.memcpy.p0.p0.i32(ptr align 4 [[AGG_TMP]], ptr align 4 @cell, i32 16, i1 false)
; CHECK-NEXT: call void @check(ptr byval(%struct.s) [[AGG_TMP]]) ; CHECK-NEXT: call void @check(ptr [[AGG_TMP]])
; CHECK-NEXT: ret void ; CHECK-NEXT: ret void
; ;
entry: entry:
%agg.tmp = alloca %struct.s, align 4 %agg.tmp = alloca %struct.s, align 4
store i32 99, ptr getelementptr inbounds (%struct.s, ptr @cell, i32 0, i32 1), align 4 store i32 99, ptr getelementptr inbounds (%struct.s, ptr @cell, i32 0, i32 1), align 4
call void @llvm.memcpy.p0.p0.i32(ptr align 1 @cell, ptr align 1 @.str, i32 11, i1 false) call void @llvm.memcpy.p0.p0.i32(ptr align 1 @cell, ptr align 1 @.str, i32 11, i1 false)
call void @llvm.memcpy.p0.p0.i32(ptr align 4 %agg.tmp, ptr align 4 @cell, i32 16, i1 false) call void @llvm.memcpy.p0.p0.i32(ptr align 4 %agg.tmp, ptr align 4 @cell, i32 16, i1 false)
call void @check(ptr byval(%struct.s) %agg.tmp) call void @check(ptr %agg.tmp)
ret void ret void
} }