[llvm] r328748 - [MemorySSA] Consider callsite args for hashing and equality.

[George Burgess IV via llvm-commits]

Hello Tom and Hal,

I would like approval to pick this (and its follow-up, which I'll ping you
both about in a sec :) ) into the 6.0 release branch. It fixes a
misoptimization that dates back to 5.0, but since 5.0.2rc2 was just cut, I
think it's best to leave that as-is.

Thanks,
George

On Wed, Mar 28, 2018 at 5:54 PM, George Burgess IV via llvm-commits <
llvm-commits at lists.llvm.org> wrote:

> Author: gbiv
> Date: Wed Mar 28 17:54:39 2018
> New Revision: 328748
>
> URL: http://llvm.org/viewvc/llvm-project?rev=328748&view=rev
> Log:
> [MemorySSA] Consider callsite args for hashing and equality.
>
> We use a `DenseMap<MemoryLocOrCall, MemlocStackInfo>` to keep track of
> prior work when optimizing uses in MemorySSA. Because we weren't
> accounting for callsite arguments in either the hash code or equality
> tests for `MemoryLocOrCall`s, we optimized uses too aggressively in
> some rare cases.
>
> Fix by Daniel Berlin.
>
> Should fix PR36883.
>
> Added:
>     llvm/trunk/test/Analysis/MemorySSA/pr36883.ll
> Modified:
>     llvm/trunk/lib/Analysis/MemorySSA.cpp
>
> Modified: llvm/trunk/lib/Analysis/MemorySSA.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Analysis/
> MemorySSA.cpp?rev=328748&r1=328747&r2=328748&view=diff
> ============================================================
> ==================
> --- llvm/trunk/lib/Analysis/MemorySSA.cpp (original)
> +++ llvm/trunk/lib/Analysis/MemorySSA.cpp Wed Mar 28 17:54:39 2018
> @@ -153,9 +153,14 @@ public:
>      if (IsCall != Other.IsCall)
>        return false;
>
> -    if (IsCall)
> -      return CS.getCalledValue() == Other.CS.getCalledValue();
> -    return Loc == Other.Loc;
> +    if (!IsCall)
> +      return Loc == Other.Loc;
> +
> +    if (CS.getCalledValue() != Other.CS.getCalledValue())
> +      return false;
> +
> +    assert(CS.arg_size() == Other.CS.arg_size());
> +    return std::equal(CS.arg_begin(), CS.arg_end(), Other.CS.arg_begin());
>    }
>
>  private:
> @@ -179,12 +184,18 @@ template <> struct DenseMapInfo<MemoryLo
>    }
>
>    static unsigned getHashValue(const MemoryLocOrCall &MLOC) {
> -    if (MLOC.IsCall)
> -      return hash_combine(MLOC.IsCall,
> -                          DenseMapInfo<const Value *>::getHashValue(
> -                              MLOC.getCS().getCalledValue()));
> -    return hash_combine(
> -        MLOC.IsCall, DenseMapInfo<MemoryLocation>::
> getHashValue(MLOC.getLoc()));
> +    if (!MLOC.IsCall)
> +      return hash_combine(
> +          MLOC.IsCall,
> +          DenseMapInfo<MemoryLocation>::getHashValue(MLOC.getLoc()));
> +
> +    hash_code hash =
> +        hash_combine(MLOC.IsCall, DenseMapInfo<const Value
> *>::getHashValue(
> +                                      MLOC.getCS().getCalledValue()));
> +
> +    for (const Value *Arg : MLOC.getCS().args())
> +      hash = hash_combine(hash, DenseMapInfo<const Value
> *>::getHashValue(Arg));
> +    return hash;
>    }
>
>    static bool isEqual(const MemoryLocOrCall &LHS, const MemoryLocOrCall
> &RHS) {
>
> Added: llvm/trunk/test/Analysis/MemorySSA/pr36883.ll
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/test/Analysis
> /MemorySSA/pr36883.ll?rev=328748&view=auto
> ============================================================
> ==================
> --- llvm/trunk/test/Analysis/MemorySSA/pr36883.ll (added)
> +++ llvm/trunk/test/Analysis/MemorySSA/pr36883.ll Wed Mar 28 17:54:39 2018
> @@ -0,0 +1,38 @@
> +; RUN: opt -basicaa -memoryssa -analyze < %s 2>&1 -S | FileCheck %s
> +; RUN: opt -aa-pipeline=basic-aa -passes='print<memoryssa>,verify<memoryssa>'
> -S < %s 2>&1 | FileCheck %s
> +;
> +; We weren't properly considering the args in callsites in equality or
> hashing.
> +
> +target triple = "armv7-dcg-linux-gnueabi"
> +
> +; CHECK-LABEL: define <8 x i16> @vpx_idct32_32_neon
> +define <8 x i16> @vpx_idct32_32_neon(i8* %p, <8 x i16> %v) {
> +entry:
> +; CHECK: MemoryUse(liveOnEntry)
> +  %load1 = call <8 x i16> @llvm.arm.neon.vld1.v8i16.p0i8(i8* %p, i32 2)
> #4 ; load CSE replacement
> +
> +; CHECK: 1 = MemoryDef(liveOnEntry)
> +  call void @llvm.arm.neon.vst1.p0i8.v8i16(i8* %p, <8 x i16> %v, i32 2)
> #4 ; clobber
> +
> +  %p_next = getelementptr inbounds i8, i8* %p, i32 16
> +; CHECK: MemoryUse(liveOnEntry)
> +  %load2 = call <8 x i16> @llvm.arm.neon.vld1.v8i16.p0i8(i8* %p_next,
> i32 2) #4 ; non-aliasing load needed to trigger bug
> +
> +; CHECK: MemoryUse(1)
> +  %load3 = call <8 x i16> @llvm.arm.neon.vld1.v8i16.p0i8(i8* %p, i32 2)
> #4 ; load CSE removed
> +
> +  %add = add <8 x i16> %load1, %load2
> +  %ret = add <8 x i16> %add, %load3
> +  ret <8 x i16> %ret
> +}
> +
> +; Function Attrs: argmemonly nounwind readonly
> +declare <8 x i16> @llvm.arm.neon.vld1.v8i16.p0i8(i8*, i32) #2
> +
> +; Function Attrs: argmemonly nounwind
> +declare void @llvm.arm.neon.vst1.p0i8.v8i16(i8*, <8 x i16>, i32) #1
> +
> +attributes #1 = { argmemonly nounwind }
> +attributes #2 = { argmemonly nounwind readonly }
> +attributes #3 = { nounwind readnone }
> +attributes #4 = { nounwind }
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180330/28f00562/attachment.html>