[llvm] r328748 - [MemorySSA] Consider callsite args for hashing and equality.
George Burgess IV via llvm-commits
llvm-commits at lists.llvm.org
Mon Apr 9 19:13:19 PDT 2018
Thanks, Hal!
Is this OK with you, too, Tom?
George
On Fri, Mar 30, 2018 at 1:33 PM, Hal Finkel <hfinkel at anl.gov> wrote:
>
> On 03/30/2018 03:04 PM, George Burgess IV wrote:
>
> 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.
>
>
> I agree. Pulling this (along with r328755) into the release branch is
> appropriate.
>
> -Hal
>
>
>
> 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
>>
>
>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20180409/db31fc86/attachment.html>
More information about the llvm-commits
mailing list