[PATCH] D35035: [InstCombine] Prevent memcpy generation for small data size

[JF Bastien via Phabricator via llvm-commits]

jfb added a comment.

I don't understand why it makes sense to tie this to the larger integer type. I understand that targets might want what you're doing, but tying it to the largest integer type instead of having a separate target-specific value seems odd.
How does this interact with memcpyopt?
Which targets are affected by this change?
Can you please provide size and performance numbers for relevant targets?



================
Comment at: lib/Transforms/InstCombine/InstCombineCalls.cpp:139
+  // largest legal integer size.
+  unsigned LargestInt = DL.getLargestLegalIntTypeSizeInBits();
+  if (!LargestInt || Size > LargestInt)
----------------
Why does this make sense? I don't understand why we'd want to tie this to the largest legal integer type, and not have it be its own target parameter.


================
Comment at: test/DebugInfo/X86/array2.ll:21
+; CHECK: tail call void @llvm.dbg.value(metadata i8** [[ARGV:%.*]], i64 0, metadata !24, metadata !12), !dbg !23
+; CHECK: tail call void @llvm.dbg.value(metadata i32 42, metadata ![[ARRAY:[0-9]+]], metadata !DIExpression(DW_OP_LLVM_fragment, 0, 32))
 ; CHECK: ![[ARRAY]] = !DILocalVariable(name: "array",{{.*}} line: 6
----------------
Why change this?


================
Comment at: test/Transforms/InstCombine/2007-10-10-EliminateMemCpy.ll:3
 ; RUN: opt < %s -O3 -S | not grep xyz
-target datalayout = "E-p:64:64:64-a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-v64:64:64-v128:128:128"
+target datalayout = "E-p:64:64:64-a0:0:8-f32:32:32-f64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:32:64-v64:64:64-v128:128:128-n32"
 
----------------
Why change this?


================
Comment at: test/Transforms/InstCombine/alloca.ll:147
   %argmem = alloca inalloca <{ %struct_type }>
-; CHECK: alloca inalloca i64, align 8
   %0 = getelementptr inbounds <{ %struct_type }>, <{ %struct_type }>* %argmem, i32 0, i32 0
----------------
This test checks that the `inalloca` remains. Change the test to still test the same thing, don't just delete the `CHECK`.
https://bugs.llvm.org/show_bug.cgi?id=19569


https://reviews.llvm.org/D35035