<div dir="ltr"><div>Hi Adrian. I assumed this thread was kind of dead and submitted a better patch (you'll find it attached).<br><br></div>This updated patch test more in depth the struct case. I don't have an array test case ready. I can add one if that is really necessary.<br>
</div><div class="gmail_extra"><br><br><div class="gmail_quote">2014-02-11 15:04 GMT-08:00 Adrian Prantl <span dir="ltr"><<a href="mailto:aprantl@apple.com" target="_blank">aprantl@apple.com</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
Thanks for adding the test! It looks like this only tests the struct case? There are probably others more qualified to comment on the GVN-related changes.<br>
<span class="HOEnZb"><font color="#888888"><br>
-- adrian<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
On Feb 7, 2014, at 21:02, deadal nix <<a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a>> wrote:<br>
<br>
> Here is the updated patch with test case included:<br>
><br>
> diff --git a/lib/Transforms/Scalar/GVN.cpp b/lib/Transforms/Scalar/GVN.cpp<br>
> index f350b9b..c8a0ec0 100644<br>
> --- a/lib/Transforms/Scalar/GVN.cpp<br>
> +++ b/lib/Transforms/Scalar/GVN.cpp<br>
> @@ -996,11 +996,6 @@ static int AnalyzeLoadFromClobberingWrite(Type *LoadTy, Value *LoadPtr,<br>
> static int AnalyzeLoadFromClobberingStore(Type *LoadTy, Value *LoadPtr,<br>
> StoreInst *DepSI,<br>
> const DataLayout &TD) {<br>
> - // Cannot handle reading from store of first-class aggregate yet.<br>
> - if (DepSI->getValueOperand()->getType()->isStructTy() ||<br>
> - DepSI->getValueOperand()->getType()->isArrayTy())<br>
> - return -1;<br>
> -<br>
> Value *StorePtr = DepSI->getPointerOperand();<br>
> uint64_t StoreSize =TD.getTypeSizeInBits(DepSI->getValueOperand()->getType());<br>
> return AnalyzeLoadFromClobberingWrite(LoadTy, LoadPtr,<br>
> @@ -1097,6 +1092,25 @@ static Value *GetStoreValueForLoad(Value *SrcVal, unsigned Offset,<br>
><br>
> IRBuilder<> Builder(InsertPt->getParent(), InsertPt);<br>
><br>
> + // When facing an Aggregate, we have to find the element to fetch from.<br>
> + while (SrcVal->getType()->isAggregateType()) {<br>
> + if (StructType *ST = dyn_cast<StructType>(SrcVal->getType())) {<br>
> + const StructLayout &Layout = *TD.getStructLayout(ST);<br>
> + unsigned i = 0;<br>
> + for (unsigned cd = 0, e = ST->getNumElements(); cd != e; ++cd) {<br>
> + if (Layout.getElementOffsetInBits(cd) > Offset) break;<br>
> + i = cd;<br>
> + }<br>
> +<br>
> + SrcVal = Builder.CreateExtractValue(SrcVal, i);<br>
> + Offset -= Layout.getElementOffsetInBits(i);<br>
> + } else if (ArrayType *AT = dyn_cast<ArrayType>(SrcVal->getType())) {<br>
> + uint64_t EltSize = TD.getTypeAllocSizeInBits(AT->getElementType());<br>
> + SrcVal = Builder.CreateExtractValue(SrcVal, Offset / EltSize);<br>
> + Offset %= EltSize;<br>
> + }<br>
> + }<br>
> +<br>
> // Compute which bits of the stored value are being used by the load. Convert<br>
> // to an integer type to start with.<br>
> if (SrcVal->getType()->getScalarType()->isPointerTy())<br>
> diff --git a/test/Transforms/GVN/aggregate_store.ll b/test/Transforms/GVN/aggregate_store.ll<br>
> new file mode 100644<br>
> index 0000000..fc8aab4<br>
> --- /dev/null<br>
> +++ b/test/Transforms/GVN/aggregate_store.ll<br>
> @@ -0,0 +1,36 @@<br>
> +; RUN: opt -gvn -S < %s | FileCheck %s<br>
> +<br>
> +target datalayout =<br>
> +"e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-f128:128:128-n8:16:32:64"<br>
> +target triple = "x86_64-unknown-linux-gnu"<br>
> +<br>
> +%A__vtbl = type { i8*, i32 (%A*)* }<br>
> +%A = type { %A__vtbl* }<br>
> +<br>
> +@A__vtblZ = constant %A__vtbl { i8* null, i32 (%A*)* @A.foo }<br>
> +@A__initZ = constant %A { %A__vtbl* @A__vtblZ }<br>
> +<br>
> +declare i8* @allocmemory(i64)<br>
> +<br>
> +define i32 @f1() {<br>
> +body:<br>
> + %0 = tail call i8* @allocmemory(i64 8)<br>
> + %1 = bitcast i8* %0 to %A*<br>
> +; CHECK: store %A<br>
> + store %A { %A__vtbl* @A__vtblZ }, %A* %1, align 8<br>
> + %2 = getelementptr inbounds %A* %1, i32 0, i32 0<br>
> +; CHECK-NOT: load %A__vtbl*<br>
> + %vtbl.i = load %A__vtbl** %2, align 8<br>
> + %3 = getelementptr inbounds %A__vtbl* %vtbl.i, i64 0, i32 1<br>
> +; CHECK-NOT: load i32 (%A*)*<br>
> + %4 = load i32 (%A*)** %3, align 8<br>
> +; CHECK: tail call i32 @A.foo(%A* %1)<br>
> + %5 = tail call i32 %4(%A* %1)<br>
> + ret i32 %5<br>
> +}<br>
> +<br>
> +define i32 @A.foo(%A* nocapture %this) nounwind readnone {<br>
> +body:<br>
> + ret i32 42<br>
> +}<br>
> +<br>
><br>
><br>
><br>
> 2014-02-07 10:01 GMT-08:00 Adrian Prantl <<a href="mailto:aprantl@apple.com">aprantl@apple.com</a>>:<br>
><br>
> On Feb 6, 2014, at 22:52, deadal nix <<a href="mailto:deadalnix@gmail.com">deadalnix@gmail.com</a>> wrote:<br>
> ><br>
> > I have some test cases, but I'm not sure how the LLVM test suite work and what is the right way to add them. I'll need some directions here to complete the diff.Anyway, the code :<br>
><br>
> To test a change to GVN it would be best to have a .ll file with a RUN: line that invokes opt (with the appropriate flags for GVN) and then CHECKs the output with FileCheck. You can grep for "RUN: opt” in test/ for some examples.<br>
> Assuming that you have source code for a testcase, you can extract an appropriate fragment from the output go -mllvm -print-after-all.<br>
><br>
> -- adrian<br>
><br>
<br>
</div></div></blockquote></div><br></div>