r218714 - Make better use of zext and sign information revert saga

[Hal Finkel via llvm-commits]

Hi Geoff, Mikhail,

Yes, as I recall, that change will fix that bug, but end up causing some other more-subtle ones. I'm okay with committing that fix as an intermediate step, but we really need to reduce whatever problems are caused by the latest proposed patch (Quentin had been looking at that) to resolve this in the long term.

We might also want to turn off looking through the casts all together until the logic can be completely fixed.

 -Hal

----- Original Message -----
> From: "Mikhail Zolotukhin" <mzolotukhin at apple.com>
> To: "Geoff Berry" <gberry at codeaurora.org>
> Cc: "Quentin Colombet" <qcolombet at apple.com>, "Hal Finkel" <hfinkel at anl.gov>, llvm-commits at lists.llvm.org
> Sent: Thursday, August 27, 2015 1:47:41 PM
> Subject: Re: r218714 - Make better use of zext and sign information revert saga
> 
> 
> Hi Geoff,
> 
> 
> Yes, we discovered that failure too (this is 473.astar, right?), you
> could find some discussion in this thread:
> http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20150824/295925.html
> 
> 
> There is no a clear solution at this point though.
> 
> 
> Thanks,
> Michael
> 
> 
> 
> 
> On Aug 27, 2015, at 11:28 AM, Geoff Berry via llvm-commits <
> llvm-commits at lists.llvm.org > wrote:
> 
> 
> Hi All,
> 
> I've tracked down a failure in SPEC2006 to BasicAliasAnalysis, and I
> believe
> the fix is part of the code touched by the repeated revert/un-revert
> cycle
> set off by the original change in the subject line above.
> Specifically, the
> patch that I believe fixes the failure is:
> 
> Value *Result = GetLinearExpression(CastOp, Scale, Offset, Extension,
> DL,
> Depth + 1, AC, DT);
> Scale = Scale.zext(OldWidth);
> - Offset = Offset.zext(OldWidth);
> +
> + // We have to sign-extend even if Extension == EK_ZeroExt as we
> can't
> + // decompose a sign extension (i.e. zext(x - 1) != zext(x) -
> zext(-1)).
> + Offset = Offset.sext(OldWidth);
> 
> Is this a known issue that someone is working to address, or a case
> of a bad
> series of reverts/un-reverts? Hal, it looks like you introduced this
> code
> in r221876. Is this bug fix part of the original patch worth checking
> in
> separately? Quentin, can you comment on whether just this part of the
> patch
> is okay w.r.t. the failures that caused you to most recently revert
> it?
> 
> Here is a test case that demonstrates the problem. Without this fix,
> basicaa returns NoAlias for the two GEPs, which causes gvn to replace
> the
> load with undef.
> 
> ; RUN: opt < %s -basicaa -aa-eval -print-all-alias-modref-info
> -disable-output 2>&1 | FileCheck %s
> target datalayout = "e-m:e-i64:64-i128:128-n32:64-S128"
> target triple = "aarch64--linux-gnu"
> 
> declare noalias i8* @malloc(i64)
> 
> ; CHECK-LABEL: gvnbadundef:
> ; CHECK: MustAlias: i8* %idx_minus1_sext_ptr, i8*
> %%idx_sext_minus1_ptr
> define i8 @gvnbadundef(i8 %val, i64 %size, i32 %idx) {
> entry:
> %mem = tail call noalias i8* @malloc(i64 %size)
> 
> ; mem[idx-1] = val
> %idx_minus1 = add nsw i32 %idx, -1
> %idx_minus1_sext = sext i32 %idx_minus1 to i64
> %idx_minus1_sext_ptr = getelementptr inbounds i8, i8* %mem, i64
> %idx_minus1_sext
> store i8 %val, i8* %idx_minus1_sext_ptr, align 1
> 
> ; return mem[idx-1]
> %idx_sext = sext i32 %idx to i64
> %idx_sext_minus1 = add nsw i64 %idx_sext, -1
> %idx_sext_minus1_ptr = getelementptr inbounds i8, i8* %mem, i64
> %idx_sext_minus1
> %load4 = load i8, i8* %idx_sext_minus1_ptr, align 1
> ret i8 %load4
> }
> 
> 
> --
> Geoff Berry
> Employee of Qualcomm Innovation Center, Inc.
> Qualcomm Innovation Center, Inc. is a member of Code Aurora Forum, a
> Linux
> Foundation Collaborative Project
> 
> 
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> https://urldefense.proofpoint.com/v2/url?u=http-3A__lists.llvm.org_cgi-2Dbin_mailman_listinfo_llvm-2Dcommits&d=BQIGaQ&c=eEvniauFctOgLOKGJOplqw&r=ygVmcuuQ1MUhRUoJm-IgPtgjmvM0byfjlHDg99vufEI&m=QWqSSQ5DxOlduXqFTxx_1fGW9_f8psq0ASbWM7wDRxQ&s=jikhMUF3xFqXCiLQUAuQy0ui_CWe_1TcDb9MQAW6OZs&e=
> 
> 

-- 
Hal Finkel
Assistant Computational Scientist
Leadership Computing Facility
Argonne National Laboratory