[MemCpyOpt] Allow variable lengths in memcpy optimizer

[ollef]

Summary

This makes the memcpy-memcpy and memcpy-memset optimizations work for
variable sizes as long as they are equal, relaxing the old restriction
that they are constant integers. If they're not equal, the old
requirement that they are constant integers with certain size
restrictions is used.

The implementation works by pushing the length tests further down in the
code, which reveals some places where it's enough that the lengths are
equal (but not necessarily constant).

[nikic]

Looks fine, I'd suggest a few more tests:

• For the two you already have, add negative tests where the size is dynamic and doesn't match.
• Add a memcpy from uninit alloca with dynamic size.

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1437 ↗

(On Diff #338919)

I'd move this check a bit higher (accessedBetween is more expensive).

[ollef]

Address review comments.

• [MemCpyOpt] Remove unused type from tests
• [MemCpyOpt] Add negative tests for non-equal variable sizes
• [MemCpyOpt] Move constant int check higher up
• [MemCpyOpt] Add test for variable sized copy from uninit memory

[ollef]

Fix spelling in file name

[ollef]

• [MemCpyOpt] Remove unused type from tests
• [MemCpyOpt] Add negative tests for non-equal variable sizes
• [MemCpyOpt] Move constant int check higher up
• [MemCpyOpt] Add test for variable sized copy from uninit memory

[xbolva00]

This comment was removed by xbolva00.

[ollef]

I believe I've addressed the reviewer comments and even managed to upload the changes. Still figuring out this Arcanist tool... 😅

[nikic]

llvm/lib/Transforms/Scalar/MemCpyOptimizer.cpp

1254 ↗

(On Diff #338966)

The constant size is only needed for the isMustAlias case below. The other one can work with a dynamic size.

[ollef]

• [MemCpyOpt] Avoid constant int check when checking for undef content

[nikic]

LGTM

[ollef]

Thanks! Can you commit this? I don't think I have commit access.

[nikic]

@ollef Could you please share the Name <email@address> to use for the commit?

[ollef]

@nikic: Please use Olle Fredriksson <[e-mail]>.

[manojgupta]

Late comment but this broke LUA (could be a mis-compile ) which further broke building sqlite. This commit was reverted in Chrome OS (https://chromium-review.googlesource.com/c/chromiumos/overlays/chromiumos-overlay/+/3066844) but seems like the reporter forgot to mention it upstream.

[manojgupta]

Chrome OS bug: https://bugs.chromium.org/p/chromium/issues/detail?id=1233594. Seems like it was tcl that was miscompiled, not LUA.

llvm/test/Transforms/MemCpyOpt/variable-sized-memcpy-uninit.ll

 ; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
 ; RUN: opt < %s -memcpyopt -S -enable-memcpyopt-memoryssa=0 | FileCheck %s
 ; RUN: opt < %s -memcpyopt -S -enable-memcpyopt-memoryssa=1 -verify-memoryssa | FileCheck %s
 target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
 
 define void @test(i64 %size) {
 ; CHECK-LABEL: @test(
 ; CHECK-NEXT:    [[SRC:%.*]] = alloca i8, i64 [[SIZE:%.*]], align 1
 ; CHECK-NEXT:    [[DST:%.*]] = alloca i8, i64 [[SIZE]], align 1
 ; CHECK-NEXT:    ret void
 ;
   %src = alloca i8, i64 %size
   %dst = alloca i8, i64 %size
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %dst, i8* align 8 %src, i64 %size, i1 false)
 
   ret void
 }
 
 define void @test2(i64 %size1, i64 %size2, i64 %cpy_size) {
 ; CHECK-LABEL: @test2(
 ; CHECK-NEXT:    [[SRC:%.*]] = alloca i8, i64 [[SIZE1:%.*]], align 1
 ; CHECK-NEXT:    [[DST:%.*]] = alloca i8, i64 [[SIZE2:%.*]], align 1
 ; CHECK-NEXT:    ret void
 ;
   %src = alloca i8, i64 %size1
   %dst = alloca i8, i64 %size2
   call void @llvm.memcpy.p0i8.p0i8.i64(i8* align 8 %dst, i8* align 8 %src, i64 %cpy_size, i1 false)
 
   ret void
 }
 
 declare void @llvm.memcpy.p0i8.p0i8.i64(i8*, i8*, i64, i1)