[llvm] r373333 - [InstCombine] sprintf(dest, "%s", str) -> memccpy(dest, str, 0, MAX)
Nick Desaulniers via llvm-commits
llvm-commits at lists.llvm.org
Wed Oct 2 10:27:51 PDT 2019
- Previous message: [llvm] r373333 - [InstCombine] sprintf(dest, "%s", str) -> memccpy(dest, str, 0, MAX)
- Next message: [llvm] r373333 - [InstCombine] sprintf(dest, "%s", str) -> memccpy(dest, str, 0, MAX)
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
Also, the kernel doesn't want to make use of -ffreestanding (except in
very specific cases) as it kneecaps other useful compiler transforms.
On Wed, Oct 2, 2019 at 10:26 AM Nick Desaulniers
<ndesaulniers at google.com> wrote:
>
> On Wed, Oct 2, 2019 at 8:41 AM Dávid Bolvanský
> <david.bolvansky at gmail.com> wrote:
> >
> > Well, memcpy is emitted only on posix/unix like systems. TLI claims
> > that Windows does not have a memccpy, so transformation XXX -> memccpy
> > is not possible. (They will have to implement it for C20).
> >
> > The linux kernel is always special case, so I think the fix should be
> > on linux kernel side, -ffreestanding / -fno-builtin-snprintf works
> > here. Same story as for "bcmp" some time ago (I dont know how that was
> > resolved).
>
> For bcmp, I added a simple alias to memcmp:
> https://github.com/ClangBuiltLinux/linux/commit/5f074f3e192f10c9fade898b9b3b8812e3d83342
>
> The kernel is compiled as -std=gnu89. Can we not emit calls to
> memccpy if LangOpt says we're < C20?
>
> >
> > >> May I ask that you copy Nick Desaulniers (on CC:) on the review for
> > your next change that introduces memccpy.
> >
> > Yeah, I will do inform them.
> >
> > st 2. 10. 2019 o 17:16 Peter Smith <peter.smith at linaro.org> napísal(a):
> > >
> > > Thanks for the response, apologies for missing the revert earlier.
> > >
> > > May I ask that you copy Nick Desaulniers (on CC:) on the review for
> > > your next change that introduces memccpy. The linux kernel does use
> > > snprintf and I'm sure they'd want some warning that their clang builds
> > > are likely to break.
> > > My understanding (https://clang.llvm.org/compatibility.html) is that
> > > clang defaults to C11, I can understand making the transformation if
> > > the standard is set to C20, however I'm not sure whether we should be
> > > doing it when the standard is C11. Anyway one for the review.
> > >
> > > Peter
> > >
> > > On Wed, 2 Oct 2019 at 16:07, Dávid Bolvanský <david.bolvansky at gmail.com> wrote:
> > > >
> > > > This was reverted a bit later. I mismeasured it and thought it is faster than memcpy and strlen solution.
> > > > Anyway, snprintf -> memccpy is worth it (30% faster). I have a patch for it, I will land it later. There are more oportunities to exploit memccpy for efficient string concatenation/copying. (You can find redhat’s article somewhere about it).
> > > >
> > > > It is a part of C20. The solution for the kernel is either use -fno-builtin-memccpy or implement it.
> > > >
> > > > The kernel
> > > >
> > > > Odoslané z iPhonu
> > > >
> > > > > Dňa 2. 10. 2019 o 15:25 užívateľ Peter Smith <peter.smith at linaro.org> napísal:
> > > > >
> > > > > This change has caused a regression in the AArch64 Linux kernel build
> > > > > (Linaro CI), I'll forward the details from the CI system of how to
> > > > > reproduce it directly via email
> > > > >
> > > > > The errors look to be from the linux kernel module tool modpost. These
> > > > > are undefined symbols coming from linux kernel modules.
> > > > > ERROR: "memccpy" [sound/pci/hda/snd-hda-codec-ca0132.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/infiniband/hw/mlx4/mlx4_ib.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/target/iscsi/iscsi_target_mod.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/nvdimm/libnvdimm.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/staging/rtl8192e/rtllib.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/i2c/busses/i2c-jz4780.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/usb/mtu3/mtu3.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/net/usb/hso.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/net/wireless/ath/ath9k/ath9k_htc.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/net/ethernet/apm/xgene-v2/xgene-enet-v2.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/net/ethernet/apm/xgene/xgene-enet.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/net/ethernet/qlogic/qed/qed.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/net/ethernet/mellanox/mlx4/mlx4_core.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/net/ethernet/cavium/liquidio/liquidio_vf.ko]
> > > > > undefined!
> > > > > ERROR: "memccpy" [drivers/net/ethernet/cavium/liquidio/liquidio.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/net/ethernet/brocade/bna/bna.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/scsi/mpt3sas/mpt3sas.ko] undefined!
> > > > > ERROR: "memccpy" [drivers/scsi/bfa/bfa.ko] undefined!
> > > > >
> > > > > I believe that memccpy is not in the C Standard so it is likely the
> > > > > Linux kernel does not provide its own definition. Is there a reason to
> > > > > use a non-standard memccpy in the transformation unless we can
> > > > > guarantee that the environment will support it?
> > > > >
> > > > > Can you take a look?
> > > > >
> > > > > Peter
> > > > >
> > > > >> On Tue, 1 Oct 2019 at 14:01, David Bolvansky via llvm-commits
> > > > >> <llvm-commits at lists.llvm.org> wrote:
> > > > >>
> > > > >> Author: xbolva00
> > > > >> Date: Tue Oct 1 06:03:10 2019
> > > > >> New Revision: 373333
> > > > >>
> > > > >> URL: http://llvm.org/viewvc/llvm-project?rev=373333&view=rev
> > > > >> Log:
> > > > >> [InstCombine] sprintf(dest, "%s", str) -> memccpy(dest, str, 0, MAX)
> > > > >>
> > > > >> Modified:
> > > > >> llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp
> > > > >> llvm/trunk/test/Transforms/InstCombine/2010-05-30-memcpy-Struct.ll
> > > > >> llvm/trunk/test/Transforms/InstCombine/sprintf-1.ll
> > > > >>
> > > > >> Modified: llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp
> > > > >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp?rev=373333&r1=373332&r2=373333&view=diff
> > > > >> ==============================================================================
> > > > >> --- llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp (original)
> > > > >> +++ llvm/trunk/lib/Transforms/Utils/SimplifyLibCalls.cpp Tue Oct 1 06:03:10 2019
> > > > >> @@ -2415,9 +2415,11 @@ Value *LibCallSimplifier::optimizePrintF
> > > > >> }
> > > > >>
> > > > >> Value *LibCallSimplifier::optimizeSPrintFString(CallInst *CI, IRBuilder<> &B) {
> > > > >> + Value *Dst = CI->getArgOperand(0);
> > > > >> + Value *FmtStr = CI->getArgOperand(1);
> > > > >> // Check for a fixed format string.
> > > > >> StringRef FormatStr;
> > > > >> - if (!getConstantStringInfo(CI->getArgOperand(1), FormatStr))
> > > > >> + if (!getConstantStringInfo(FmtStr, FormatStr))
> > > > >> return nullptr;
> > > > >>
> > > > >> // If we just have a format string (nothing else crazy) transform it.
> > > > >> @@ -2428,9 +2430,10 @@ Value *LibCallSimplifier::optimizeSPrint
> > > > >> return nullptr; // we found a format specifier, bail out.
> > > > >>
> > > > >> // sprintf(str, fmt) -> llvm.memcpy(align 1 str, align 1 fmt, strlen(fmt)+1)
> > > > >> - B.CreateMemCpy(CI->getArgOperand(0), 1, CI->getArgOperand(1), 1,
> > > > >> - ConstantInt::get(DL.getIntPtrType(CI->getContext()),
> > > > >> - FormatStr.size() + 1)); // Copy the null byte.
> > > > >> + B.CreateMemCpy(
> > > > >> + Dst, 1, FmtStr, 1,
> > > > >> + ConstantInt::get(DL.getIntPtrType(CI->getContext()),
> > > > >> + FormatStr.size() + 1)); // Copy the null byte.
> > > > >> return ConstantInt::get(CI->getType(), FormatStr.size());
> > > > >> }
> > > > >>
> > > > >> @@ -2440,13 +2443,14 @@ Value *LibCallSimplifier::optimizeSPrint
> > > > >> CI->getNumArgOperands() < 3)
> > > > >> return nullptr;
> > > > >>
> > > > >> + Value *Str = CI->getArgOperand(2);
> > > > >> // Decode the second character of the format string.
> > > > >> if (FormatStr[1] == 'c') {
> > > > >> // sprintf(dst, "%c", chr) --> *(i8*)dst = chr; *((i8*)dst+1) = 0
> > > > >> - if (!CI->getArgOperand(2)->getType()->isIntegerTy())
> > > > >> + if (!Str->getType()->isIntegerTy())
> > > > >> return nullptr;
> > > > >> - Value *V = B.CreateTrunc(CI->getArgOperand(2), B.getInt8Ty(), "char");
> > > > >> - Value *Ptr = castToCStr(CI->getArgOperand(0), B);
> > > > >> + Value *V = B.CreateTrunc(Str, B.getInt8Ty(), "char");
> > > > >> + Value *Ptr = castToCStr(Dst, B);
> > > > >> B.CreateStore(V, Ptr);
> > > > >> Ptr = B.CreateGEP(B.getInt8Ty(), Ptr, B.getInt32(1), "nul");
> > > > >> B.CreateStore(B.getInt8(0), Ptr);
> > > > >> @@ -2455,17 +2459,30 @@ Value *LibCallSimplifier::optimizeSPrint
> > > > >> }
> > > > >>
> > > > >> if (FormatStr[1] == 's') {
> > > > >> - // sprintf(dest, "%s", str) -> llvm.memcpy(align 1 dest, align 1 str,
> > > > >> - // strlen(str)+1)
> > > > >> - if (!CI->getArgOperand(2)->getType()->isPointerTy())
> > > > >> + if (!Str->getType()->isPointerTy())
> > > > >> return nullptr;
> > > > >> +
> > > > >> + // sprintf(dest, "%s", str) -> memccpy(dest, str, 0, MAX)
> > > > >> + if (CI->use_empty()) {
> > > > >> + Value *S = (Dst->getType() == Str->getType())
> > > > >> + ? Str
> > > > >> + : B.CreateBitCast(Str, Dst->getType());
> > > > >> + unsigned SizeTyBitwidth =
> > > > >> + DL.getIntPtrType(CI->getContext())->getPrimitiveSizeInBits();
> > > > >> + Value *NewSize = ConstantInt::get(B.getIntNTy(SizeTyBitwidth),
> > > > >> + APInt::getMaxValue(SizeTyBitwidth));
> > > > >> + emitMemCCpy(Dst, S, B.getInt32('\0'), NewSize, B, TLI);
> > > > >> + return Dst;
> > > > >> + }
> > > > >>
> > > > >> - Value *Len = emitStrLen(CI->getArgOperand(2), B, DL, TLI);
> > > > >> + // sprintf(dest, "%s", str) -> llvm.memcpy(align 1 dest, align 1 str,
> > > > >> + // strlen(str)+1)
> > > > >> + Value *Len = emitStrLen(Str, B, DL, TLI);
> > > > >> if (!Len)
> > > > >> return nullptr;
> > > > >> Value *IncLen =
> > > > >> B.CreateAdd(Len, ConstantInt::get(Len->getType(), 1), "leninc");
> > > > >> - B.CreateMemCpy(CI->getArgOperand(0), 1, CI->getArgOperand(2), 1, IncLen);
> > > > >> + B.CreateMemCpy(Dst, 1, Str, 1, IncLen);
> > > > >>
> > > > >> // The sprintf result is the unincremented number of bytes in the string.
> > > > >> return B.CreateIntCast(Len, CI->getType(), false);
> > > > >>
> > > > >> Modified: llvm/trunk/test/Transforms/InstCombine/2010-05-30-memcpy-Struct.ll
> > > > >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/2010-05-30-memcpy-Struct.ll?rev=373333&r1=373332&r2=373333&view=diff
> > > > >> ==============================================================================
> > > > >> --- llvm/trunk/test/Transforms/InstCombine/2010-05-30-memcpy-Struct.ll (original)
> > > > >> +++ llvm/trunk/test/Transforms/InstCombine/2010-05-30-memcpy-Struct.ll Tue Oct 1 06:03:10 2019
> > > > >> @@ -1,3 +1,4 @@
> > > > >> +; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
> > > > >> ; RUN: opt -instcombine -S < %s | FileCheck %s
> > > > >> ; PR7265
> > > > >>
> > > > >> @@ -9,10 +10,14 @@ target triple = "x86_64-unknown-linux-gn
> > > > >> @.str = private constant [3 x i8] c"%s\00"
> > > > >>
> > > > >> define void @CopyEventArg(%union.anon* %ev) nounwind {
> > > > >> +; CHECK-LABEL: @CopyEventArg(
> > > > >> +; CHECK-NEXT: entry:
> > > > >> +; CHECK-NEXT: [[TMP0:%.*]] = bitcast %union.anon* [[EV:%.*]] to i8*
> > > > >> +; CHECK-NEXT: [[MEMCCPY:%.*]] = call i8* @memccpy(i8* undef, i8* [[TMP0]], i32 0, i64 -1)
> > > > >> +; CHECK-NEXT: ret void
> > > > >> +;
> > > > >> entry:
> > > > >> %call = call i32 (i8*, i8*, ...) @sprintf(i8* undef, i8* getelementptr inbounds ([3 x i8], [3 x i8]* @.str, i64 0, i64 0), %union.anon* %ev) nounwind
> > > > >> -; CHECK: bitcast %union.anon* %ev to i8*
> > > > >> -; CHECK: call void @llvm.memcpy.p0i8.p0i8.i64
> > > > >> ret void
> > > > >> }
> > > > >>
> > > > >>
> > > > >> Modified: llvm/trunk/test/Transforms/InstCombine/sprintf-1.ll
> > > > >> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Transforms/InstCombine/sprintf-1.ll?rev=373333&r1=373332&r2=373333&view=diff
> > > > >> ==============================================================================
> > > > >> --- llvm/trunk/test/Transforms/InstCombine/sprintf-1.ll (original)
> > > > >> +++ llvm/trunk/test/Transforms/InstCombine/sprintf-1.ll Tue Oct 1 06:03:10 2019
> > > > >> @@ -85,15 +85,11 @@ define void @test_simplify4(i8* %dst) {
> > > > >>
> > > > >> define void @test_simplify5(i8* %dst, i8* %str) {
> > > > >> ; CHECK-LABEL: @test_simplify5(
> > > > >> -; CHECK-NEXT: [[STRLEN:%.*]] = call i32 @strlen(i8* nonnull dereferenceable(1) [[STR:%.*]])
> > > > >> -; CHECK-NEXT: [[LENINC:%.*]] = add i32 [[STRLEN]], 1
> > > > >> -; CHECK-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 1 [[DST:%.*]], i8* align 1 [[STR]], i32 [[LENINC]], i1 false)
> > > > >> +; CHECK-NEXT: [[MEMCCPY:%.*]] = call i8* @memccpy(i8* [[DST:%.*]], i8* [[STR:%.*]], i32 0, i32 -1)
> > > > >> ; CHECK-NEXT: ret void
> > > > >> ;
> > > > >> ; CHECK-IPRINTF-LABEL: @test_simplify5(
> > > > >> -; CHECK-IPRINTF-NEXT: [[STRLEN:%.*]] = call i32 @strlen(i8* nonnull dereferenceable(1) [[STR:%.*]])
> > > > >> -; CHECK-IPRINTF-NEXT: [[LENINC:%.*]] = add i32 [[STRLEN]], 1
> > > > >> -; CHECK-IPRINTF-NEXT: call void @llvm.memcpy.p0i8.p0i8.i32(i8* align 1 [[DST:%.*]], i8* align 1 [[STR]], i32 [[LENINC]], i1 false)
> > > > >> +; CHECK-IPRINTF-NEXT: [[MEMCCPY:%.*]] = call i8* @memccpy(i8* [[DST:%.*]], i8* [[STR:%.*]], i32 0, i32 -1)
> > > > >> ; CHECK-IPRINTF-NEXT: ret void
> > > > >> ;
> > > > >> %fmt = getelementptr [3 x i8], [3 x i8]* @percent_s, i32 0, i32 0
> > > > >>
> > > > >>
> > > > >> _______________________________________________
> > > > >> llvm-commits mailing list
> > > > >> llvm-commits at lists.llvm.org
> > > > >> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
>
>
> --
> Thanks,
> ~Nick Desaulniers
--
Thanks,
~Nick Desaulniers
- Previous message: [llvm] r373333 - [InstCombine] sprintf(dest, "%s", str) -> memccpy(dest, str, 0, MAX)
- Next message: [llvm] r373333 - [InstCombine] sprintf(dest, "%s", str) -> memccpy(dest, str, 0, MAX)
- Messages sorted by:
[ date ]
[ thread ]
[ subject ]
[ author ]
More information about the llvm-commits
mailing list