From b778cbc0c8f1d970ae30e0ffb4eed17ca2bb294a Mon Sep 17 00:00:00 2001 From: Chandler Carruth Date: Thu, 22 Jan 2015 05:08:12 +0000 Subject: [PATCH] [canonicalize] Teach InstCombine to canonicalize loads which are only ever stored to always use a legal integer type if one is available. Regardless of whether this particular type is good or bad, it ensures we don't get weird differences in generated code (and resulting performance) from "equivalent" patterns that happen to end up using a slightly different type. After some discussion on llvmdev it seems everyone generally likes this canonicalization. However, there may be some parts of LLVM that handle it poorly and need to be fixed. I have at least verified that this doesn't impede GVN and instcombine's store-to-load forwarding powers in any obvious cases. Subtle cases are exactly what we need te flush out if they remain. Also note that this IR pattern should already be hitting LLVM from Clang at least because it is exactly the IR which would be produced if you used memcpy to copy a pointer or floating point between memory instead of a variable. git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@226781 91177308-0d34-0410-b5e6-96231b3b80d8 --- .../InstCombineLoadStoreAlloca.cpp | 29 +++++++++++ test/Transforms/InstCombine/load.ll | 52 ++++++++++++++++++- .../InstCombine/struct-assign-tbaa.ll | 4 +- 3 files changed, 82 insertions(+), 3 deletions(-) diff --git a/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp b/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp index 56212b04eb0..54b1af8ea79 100644 --- a/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp +++ b/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp @@ -417,6 +417,35 @@ static Instruction *combineLoadToOperationType(InstCombiner &IC, LoadInst &LI) { if (LI.use_empty()) return nullptr; + Type *Ty = LI.getType(); + + // Try to canonicalize loads which are only ever stored to operate over + // integers instead of any other type. We only do this when the loaded type + // is sized and has a size exactly the same as its store size and the store + // size is a legal integer type. + const DataLayout *DL = IC.getDataLayout(); + if (!Ty->isIntegerTy() && Ty->isSized() && DL && + DL->isLegalInteger(DL->getTypeStoreSizeInBits(Ty)) && + DL->getTypeStoreSizeInBits(Ty) == DL->getTypeSizeInBits(Ty)) { + if (std::all_of(LI.user_begin(), LI.user_end(), [&LI](User *U) { + auto *SI = dyn_cast(U); + return SI && SI->getPointerOperand() != &LI; + })) { + LoadInst *NewLoad = combineLoadToNewType( + IC, LI, + Type::getIntNTy(LI.getContext(), DL->getTypeStoreSizeInBits(Ty))); + // Replace all the stores with stores of the newly loaded value. + for (auto UI = LI.user_begin(), UE = LI.user_end(); UI != UE;) { + auto *SI = cast(*UI++); + IC.Builder->SetInsertPoint(SI); + combineStoreToNewValue(IC, *SI, NewLoad); + IC.EraseInstFromFunction(*SI); + } + assert(LI.use_empty() && "Failed to remove all users of the load!"); + // Return the old load so the combiner can delete it safely. + return &LI; + } + } // Fold away bit casts of the loaded value by loading the desired type. if (LI.hasOneUse()) diff --git a/test/Transforms/InstCombine/load.ll b/test/Transforms/InstCombine/load.ll index b4b7558a1a0..700797ab22c 100644 --- a/test/Transforms/InstCombine/load.ll +++ b/test/Transforms/InstCombine/load.ll @@ -2,7 +2,7 @@ ; This test makes sure that these instructions are properly eliminated. -target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128" +target datalayout = "e-m:e-p:64:64:64-i64:64-f80:128-n8:16:32:64-S128" @X = constant i32 42 ; [#uses=2] @X2 = constant i32 47 ; [#uses=1] @@ -150,3 +150,53 @@ define i8 @test15(i8 %x, i32 %y) { %r = load i8* %g.i8 ret i8 %r } + +define void @test16(i8* %x, i8* %a, i8* %b, i8* %c) { +; Check that we canonicalize loads which are only stored to use integer types +; when there is a valid integer type. +; CHECK-LABEL: @test16( +; CHECK: %[[L1:.*]] = load i32* +; CHECK-NOT: load +; CHECK: store i32 %[[L1]], i32* +; CHECK: store i32 %[[L1]], i32* +; CHECK-NOT: store +; CHECK: %[[L1:.*]] = load i32* +; CHECK-NOT: load +; CHECK: store i32 %[[L1]], i32* +; CHECK: store i32 %[[L1]], i32* +; CHECK-NOT: store +; CHECK: ret + +entry: + %x.cast = bitcast i8* %x to float* + %a.cast = bitcast i8* %a to float* + %b.cast = bitcast i8* %b to float* + %c.cast = bitcast i8* %c to i32* + + %x1 = load float* %x.cast + store float %x1, float* %a.cast + store float %x1, float* %b.cast + + %x2 = load float* %x.cast + store float %x2, float* %b.cast + %x2.cast = bitcast float %x2 to i32 + store i32 %x2.cast, i32* %c.cast + + ret void +} + +define void @test17(i8** %x, i8 %y) { +; Check that in cases similar to @test16 we don't try to rewrite a load when +; its only use is a store but it is used as the pointer to that store rather +; than the value. +; +; CHECK-LABEL: @test17( +; CHECK: %[[L:.*]] = load i8** +; CHECK: store i8 %y, i8* %[[L]] + +entry: + %x.load = load i8** %x + store i8 %y, i8* %x.load + + ret void +} diff --git a/test/Transforms/InstCombine/struct-assign-tbaa.ll b/test/Transforms/InstCombine/struct-assign-tbaa.ll index 4e4b3f2a1a8..e949640d261 100644 --- a/test/Transforms/InstCombine/struct-assign-tbaa.ll +++ b/test/Transforms/InstCombine/struct-assign-tbaa.ll @@ -10,8 +10,8 @@ declare void @llvm.memcpy.p0i8.p0i8.i64(i8* nocapture, i8* nocapture, i64, i32, %struct.test1 = type { float } ; CHECK: @test -; CHECK: %2 = load float* %0, align 4, !tbaa !0 -; CHECK: store float %2, float* %1, align 4, !tbaa !0 +; CHECK: %[[LOAD:.*]] = load i32* %{{.*}}, align 4, !tbaa !0 +; CHECK: store i32 %[[LOAD:.*]], i32* %{{.*}}, align 4, !tbaa !0 ; CHECK: ret define void @test1(%struct.test1* nocapture %a, %struct.test1* nocapture %b) { entry: -- 2.34.1