[llvm] 8d20f2c - Revert "[CodeGenPrepare] Fix isIVIncrement (PR49466)"
Jordan Rupprecht via llvm-commits
llvm-commits at lists.llvm.org
Fri Mar 12 14:59:16 PST 2021
On Fri, Mar 12, 2021 at 4:29 PM Philip Reames <listmail at philipreames.com>
wrote:
> Jordan,
>
> Please revert this change.
>
> There's a couple of problems here:
>
> - The change reverted looks obviously innocent. (e.g. it's bailing
> out of a transform slightly more often)
>
> I don't disagree that it "looks obviously innocent", but the proof is in
the pudding: the reproducer I posted compiles ~instantly at one commit
prior, and times out at the culprit commit. A change "looking" good should
never be a basis for saying it must be correct and should not be reverted,
especially when there is evidence it is a problem. At least, that's my
personal opinion, but I should think that's a fairly basic and widely held
belief.
>
> - The change reverted was itself fixing a functional bug. At a
> minimum, we'd need a larger revert to get ToT back to a sane state.
>
> The fix is introducing a different bug, and one which seems more
widespread. At the very least, we haven't observed any of the crashes
mentioned in PR49466, but we did notice a compile timeout in several
different compilation units.
>
> - It is generally considered reasonable to provide a test case and
> wait a bit before reverting, at least once the patch is more than a few
> hours old.
>
> Sorry about that. I'll concede that I got a little trigger happy here, but
I was hoping that would be waived by the fact that I gave a simple,
concrete reproducer.
>
> - Failure to provide an IR test case.
>
> (ditto, but see one below)
>
> - Your test case does not reproduce. Or at least, it doesn't
> reproduce when compiled with clang10 to IR and then run through (very
> recent, but without your change) ToT opt -O2. If there's something
> specific about the interaction of clang and opt ToT, reducing this down to
> a IR test case becomes particularly important.
>
> This comment is going *way* off track -- the reproducer I posted *does*
reproduce, at least for me, in the configuration I posted (a C++ source
file, and just "clang -O2"). By saying it doesn't reproduce in a mixed
configuration of an old version of clang to do the C++ -> IR combined with
a ToT version of opt -O2 to do the IR -> object file is misleading -- it's
true, but that's not at all what I was claiming, and I don't know where
it's coming from.
>
>
> Philip
>
> p.s. To be clear, I'm happy to look at your original issue this afternoon
> if you've got something I can reproduce.
>
Here's an IR reproducer with IR generated from ToT before my revert (at
dfd27ebbd0eb137c9a439b7c537bb87ba903efd3):
$ bin/clang -c /tmp/repro.cc -O1 -S -emit-llvm -o /tmp/repro.ll
$ bin/clang -c /tmp/repro.ll -O2 -o /tmp/repro.o # hangs
; ModuleID = '/tmp/repro.cc'
source_filename = "/tmp/repro.cc"
target datalayout =
"e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
%class.D = type { i64 }
%class.a = type { %class.g }
%class.g = type { i32*, i32* }
$_ZNK1aIliEixEl = comdat any
$_ZNK1aIliE1jEv = comdat any
@o = dso_local local_unnamed_addr global i32 0, align 4
@p = dso_local local_unnamed_addr global i32 0, align 4
@.str = private unnamed_addr constant [1 x i8] zeroinitializer, align 1
; Function Attrs: uwtable mustprogress
define dso_local void @_ZN1D1qERK1aIliE(%class.D* nocapture nonnull
dereferenceable(8) %0, %class.a* nonnull align 8 dereferenceable(16) %1)
local_unnamed_addr #0 align 2 {
%3 = call i64 @_ZNK1aIliEixEl(%class.a* nonnull dereferenceable(16) %1,
i64 0) #3
%4 = icmp eq i64 %3, 0
br i1 %4, label %26, label %5
5: ; preds = %2
%6 = call i64 @_ZNK1aIliE1jEv(%class.a* nonnull dereferenceable(16) %1)
%7 = icmp eq i64 %6, 0
br i1 %7, label %26, label %8
8: ; preds = %5
%9 = getelementptr inbounds %class.D, %class.D* %0, i64 0, i32 0
br label %10
10: ; preds = %8, %18
%11 = phi i64 [ 0, %8 ], [ %23, %18 ]
%12 = call i64 @_ZNK1aIliEixEl(%class.a* nonnull dereferenceable(16) %1,
i64 %11) #3
%13 = icmp eq i64 %12, 0
br i1 %13, label %18, label %14
14: ; preds = %10, %14
%15 = load i32, i32* @o, align 4, !tbaa !2
%16 = call i32 @_Z3fn1IiiEiT_T0_PKc(i32 %15, i32 0, i8* getelementptr
inbounds ([1 x i8], [1 x i8]* @.str, i64 0, i64 0))
%17 = icmp eq i32 %16, 0
br i1 %17, label %18, label %14, !llvm.loop !6
18: ; preds = %14, %10
%19 = call i64 @_ZNK1aIliEixEl(%class.a* nonnull dereferenceable(16) %1,
i64 %11) #3
%20 = load i32, i32* @p, align 4, !tbaa !2
%21 = sext i32 %20 to i64
%22 = sdiv i64 %19, %21
store i64 %22, i64* %9, align 8, !tbaa !9
%23 = add i64 %11, 1
%24 = call i64 @_ZNK1aIliE1jEv(%class.a* nonnull dereferenceable(16) %1)
%25 = icmp eq i64 %24, 0
br i1 %25, label %26, label %10, !llvm.loop !12
26: ; preds = %18, %5, %2
ret void
}
; Function Attrs: nounwind uwtable willreturn mustprogress
define linkonce_odr dso_local i64 @_ZNK1aIliEixEl(%class.a* nonnull
dereferenceable(16) %0, i64 %1) local_unnamed_addr #1 comdat align 2 {
%3 = getelementptr inbounds %class.a, %class.a* %0, i64 0, i32 0, i32 0
%4 = load i32*, i32** %3, align 8, !tbaa !13
%5 = getelementptr inbounds i32, i32* %4, i64 %1
%6 = load i32, i32* %5, align 4, !tbaa !2
%7 = sext i32 %6 to i64
ret i64 %7
}
; Function Attrs: nounwind uwtable willreturn mustprogress
define linkonce_odr dso_local i64 @_ZNK1aIliE1jEv(%class.a* nonnull
dereferenceable(16) %0) local_unnamed_addr #1 comdat align 2 {
%2 = getelementptr inbounds %class.a, %class.a* %0, i64 0, i32 0, i32 1
%3 = load i32*, i32** %2, align 8, !tbaa !16
%4 = getelementptr inbounds %class.a, %class.a* %0, i64 0, i32 0, i32 0
%5 = load i32*, i32** %4, align 8, !tbaa !13
%6 = ptrtoint i32* %3 to i64
%7 = ptrtoint i32* %5 to i64
%8 = sub i64 %6, %7
%9 = ashr exact i64 %8, 2
ret i64 %9
}
declare dso_local i32 @_Z3fn1IiiEiT_T0_PKc(i32, i32, i8*)
local_unnamed_addr #2
attributes #0 = { uwtable mustprogress "frame-pointer"="none"
"no-trapping-math"="true" "stack-protector-buffer-size"="8"
"target-cpu"="x86-64" "target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87"
"tune-cpu"="generic" }
attributes #1 = { nounwind uwtable willreturn mustprogress
"frame-pointer"="none" "no-trapping-math"="true"
"stack-protector-buffer-size"="8" "target-cpu"="x86-64"
"target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #2 = { "frame-pointer"="none" "no-trapping-math"="true"
"stack-protector-buffer-size"="8" "target-cpu"="x86-64"
"target-features"="+cx8,+fxsr,+mmx,+sse,+sse2,+x87" "tune-cpu"="generic" }
attributes #3 = { nounwind }
!llvm.module.flags = !{!0}
!llvm.ident = !{!1}
!0 = !{i32 1, !"wchar_size", i32 4}
!1 = !{!"clang version 13.0.0 (https://github.com/llvm/llvm-project.git
dfd27ebbd0eb137c9a439b7c537bb87ba903efd3)"}
!2 = !{!3, !3, i64 0}
!3 = !{!"int", !4, i64 0}
!4 = !{!"omnipotent char", !5, i64 0}
!5 = !{!"Simple C++ TBAA"}
!6 = distinct !{!6, !7, !8}
!7 = !{!"llvm.loop.mustprogress"}
!8 = !{!"llvm.loop.unroll.disable"}
!9 = !{!10, !11, i64 0}
!10 = !{!"_ZTS1D", !11, i64 0}
!11 = !{!"long", !4, i64 0}
!12 = distinct !{!12, !7, !8}
!13 = !{!14, !15, i64 0}
!14 = !{!"_ZTS1g", !15, i64 0, !15, i64 8}
!15 = !{!"any pointer", !4, i64 0}
!16 = !{!14, !15, i64 8}
> On 3/12/21 1:59 PM, Jordan Rupprecht via llvm-commits wrote:
>
> Author: Jordan Rupprecht
> Date: 2021-03-12T13:59:14-08:00
> New Revision: 8d20f2c2c66eb486ff23cc3d55a53bd840b36971
>
> URL: https://github.com/llvm/llvm-project/commit/8d20f2c2c66eb486ff23cc3d55a53bd840b36971
> DIFF: https://github.com/llvm/llvm-project/commit/8d20f2c2c66eb486ff23cc3d55a53bd840b36971.diff
>
> LOG: Revert "[CodeGenPrepare] Fix isIVIncrement (PR49466)"
>
> This reverts commit cf82700af8c658ae09b14c3d01bb1e73e48d3bd3 due to a compile timeout when building the following with `clang -O2`:
>
> ```
> template <class, class = int> class a;
> struct b {
> using d = int *;
> };
> struct e {
> using f = b::d;
> };
> class g {
> public:
> e::f h;
> e::f i;
> };
> template <class, class> class a : g {
> public:
> long j() const { return i - h; }
> long operator[](long) const noexcept;
> };
> template <class c, class k> long a<c, k>::operator[](long l) const noexcept {
> return h[l];
> }
> template <typename m, typename n> int fn1(m, n, const char *);
> int o, p;
> class D {
> void q(const a<long> &);
> long r;
> };
> void D::q(const a<long> &l) {
> int s;
> if (l[0])
> for (; l.j(); ++s) {
> if (l[s])
> while (fn1(o, 0, ""))
> ;
> r = l[s] / p;
> }
> }
> ```
>
> Added:
>
>
> Modified:
> llvm/lib/CodeGen/CodeGenPrepare.cpp
>
> Removed:
> llvm/test/CodeGen/X86/pr49466.ll
>
>
> ################################################################################
> diff --git a/llvm/lib/CodeGen/CodeGenPrepare.cpp b/llvm/lib/CodeGen/CodeGenPrepare.cpp
> index 0f698dd3b190..0b1156e2ace7 100644
> --- a/llvm/lib/CodeGen/CodeGenPrepare.cpp
> +++ b/llvm/lib/CodeGen/CodeGenPrepare.cpp
> @@ -1332,7 +1332,7 @@ getIVIncrement(const PHINode *PN, const LoopInfo *LI) {
>
> static bool isIVIncrement(const BinaryOperator *BO, const LoopInfo *LI) {
> auto *PN = dyn_cast<PHINode>(BO->getOperand(0));
> - if (!PN || LI->getLoopFor(BO->getParent()) != LI->getLoopFor(PN->getParent()))
> + if (!PN)
> return false;
> if (auto IVInc = getIVIncrement(PN, LI))
> return IVInc->first == BO;
> @@ -1347,7 +1347,6 @@ bool CodeGenPrepare::replaceMathCmpWithIntrinsic(BinaryOperator *BO,
> if (!isIVIncrement(BO, LI))
> return false;
> const Loop *L = LI->getLoopFor(BO->getParent());
> - assert(L && "L should not be null after isIVIncrement()");
> // Do not risk on moving increment into a child loop.
> if (LI->getLoopFor(Cmp->getParent()) != L)
> return false;
>
> diff --git a/llvm/test/CodeGen/X86/pr49466.ll b/llvm/test/CodeGen/X86/pr49466.ll
> deleted file mode 100644
> index 4f6574d9bbf2..000000000000
> --- a/llvm/test/CodeGen/X86/pr49466.ll
> +++ /dev/null
> @@ -1,122 +0,0 @@
> -; RUN: opt < %s -O2 -codegenprepare -S | FileCheck %s
> -
> -target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
> -target triple = "x86_64-unknown-linux-gnu"
> -
> - at b = dso_local local_unnamed_addr global i64 0, align 8
> - at c = dso_local local_unnamed_addr global i64 0, align 8
> - at d = dso_local local_unnamed_addr global i64 0, align 8
> - at e = dso_local local_unnamed_addr global i64 0, align 8
> - at f = dso_local local_unnamed_addr global i64 0, align 8
> - at g = dso_local local_unnamed_addr global i64 0, align 8
> -
> -; CHECK-LABEL: @m(
> -
> -define dso_local i32 @m() local_unnamed_addr {
> -entry:
> - %0 = load i64, i64* @f, align 8
> - %1 = inttoptr i64 %0 to i32*
> - %2 = load i64, i64* @c, align 8
> - %conv18 = trunc i64 %2 to i32
> - %cmp = icmp slt i32 %conv18, 3
> - %3 = load i64, i64* @d, align 8
> - %conv43 = trunc i64 %3 to i8
> - %tobool40.not = icmp eq i8 %conv43, 0
> - br label %for.cond
> -
> -for.cond: ; preds = %for.cond39.preheader, %entry
> - %j.0 = phi i32 [ undef, %entry ], [ %j.1.lcssa, %for.cond39.preheader ]
> - %p.0 = phi i64 [ undef, %entry ], [ %p.1.lcssa, %for.cond39.preheader ]
> - %i.0 = phi i32 [ undef, %entry ], [ %i.1.lcssa, %for.cond39.preheader ]
> - %cmp73 = icmp slt i32 %i.0, 3
> - br i1 %cmp73, label %for.body.preheader, label %for.cond39.preheader
> -
> -for.body.preheader: ; preds = %for.cond
> - br label %for.body
> -
> -for.cond1.loopexit: ; preds = %for.inc34.preheader, %for.end12
> - br i1 %cmp, label %for.body, label %for.cond39.preheader.loopexit
> -
> -for.cond39.preheader.loopexit: ; preds = %for.cond1.loopexit
> - br label %for.cond39.preheader
> -
> -for.cond39.preheader: ; preds = %for.cond39.preheader.loopexit, %for.cond
> - %j.1.lcssa = phi i32 [ %j.0, %for.cond ], [ %conv18, %for.cond39.preheader.loopexit ]
> - %p.1.lcssa = phi i64 [ %p.0, %for.cond ], [ 0, %for.cond39.preheader.loopexit ]
> - %i.1.lcssa = phi i32 [ %i.0, %for.cond ], [ %conv18, %for.cond39.preheader.loopexit ]
> - br i1 %tobool40.not, label %for.cond, label %for.inc42.preheader
> -
> -for.inc42.preheader: ; preds = %for.cond39.preheader
> - br label %for.inc42
> -
> -for.body: ; preds = %for.body.preheader, %for.cond1.loopexit
> - %l.176 = phi i8 [ %sub, %for.cond1.loopexit ], [ 0, %for.body.preheader ]
> - %p.175 = phi i64 [ 0, %for.cond1.loopexit ], [ %p.0, %for.body.preheader ]
> - %j.174 = phi i32 [ %conv18, %for.cond1.loopexit ], [ %j.0, %for.body.preheader ]
> - %tobool.not = icmp eq i32 %j.174, 0
> - br i1 %tobool.not, label %cleanup45, label %for.cond2.preheader
> -
> -for.cond2.preheader: ; preds = %for.body
> - %tobool3.not69 = icmp eq i64 %p.175, 0
> - %.pr.pre = load i64, i64* @e, align 8
> - br i1 %tobool3.not69, label %for.end12, label %for.body4.preheader
> -
> -for.body4.preheader: ; preds = %for.cond2.preheader
> - %4 = sub i64 0, %p.175
> - %xtraiter = and i64 %4, 7
> - %lcmp.mod.not = icmp eq i64 %xtraiter, 0
> - br i1 %lcmp.mod.not, label %for.body4.prol.loopexit, label %for.body4.prol.preheader
> -
> -for.body4.prol.preheader: ; preds = %for.body4.preheader
> - %5 = mul nsw i64 %xtraiter, -1
> - br label %for.body4.prol
> -
> -for.body4.prol: ; preds = %for.body4.prol.preheader, %for.body4.prol
> - %lsr.iv = phi i64 [ 0, %for.body4.prol.preheader ], [ %lsr.iv.next, %for.body4.prol ]
> - %lsr.iv.next = add nsw i64 %lsr.iv, -1
> - %prol.iter.cmp.not = icmp eq i64 %5, %lsr.iv.next
> - br i1 %prol.iter.cmp.not, label %for.body4.prol.loopexit.loopexit, label %for.body4.prol
> -
> -for.body4.prol.loopexit.loopexit: ; preds = %for.body4.prol
> - %6 = sub i64 %p.175, %lsr.iv.next
> - br label %for.body4.prol.loopexit
> -
> -for.body4.prol.loopexit: ; preds = %for.body4.prol.loopexit.loopexit, %for.body4.preheader
> - %p.270.unr = phi i64 [ %p.175, %for.body4.preheader ], [ %6, %for.body4.prol.loopexit.loopexit ]
> - %7 = icmp ugt i64 %p.175, -8
> - br i1 %7, label %for.end12, label %for.body4.preheader89
> -
> -for.body4.preheader89: ; preds = %for.body4.prol.loopexit
> - br label %for.body4
> -
> -for.body4: ; preds = %for.body4.preheader89, %for.body4
> - %p.270 = phi i64 [ %inc11.7, %for.body4 ], [ %p.270.unr, %for.body4.preheader89 ]
> - %inc11.7 = add i64 %p.270, 8
> - %tobool3.not.7 = icmp eq i64 %inc11.7, 0
> - br i1 %tobool3.not.7, label %for.end12.loopexit, label %for.body4
> -
> -for.end12.loopexit: ; preds = %for.body4
> - br label %for.end12
> -
> -for.end12: ; preds = %for.end12.loopexit, %for.body4.prol.loopexit, %for.cond2.preheader
> - %8 = load i32, i32* %1, align 4
> - %conv23 = zext i32 %8 to i64
> - %9 = load i64, i64* @b, align 8
> - %div24 = udiv i64 %9, %conv23
> - store i64 %div24, i64* @b, align 8
> - %sub = add i8 %l.176, -1
> - %tobool32.not72 = icmp eq i64 %.pr.pre, 0
> - br i1 %tobool32.not72, label %for.cond1.loopexit, label %for.inc34.preheader
> -
> -for.inc34.preheader: ; preds = %for.end12
> - store i64 0, i64* @e, align 8
> - br label %for.cond1.loopexit
> -
> -for.inc42: ; preds = %for.inc42.preheader, %for.inc42
> - br label %for.inc42
> -
> -cleanup45: ; preds = %for.body
> - %cmp13 = icmp ne i8 %l.176, 0
> - %conv16 = zext i1 %cmp13 to i32
> - ret i32 %conv16
> -}
>
>
>
> _______________________________________________
> llvm-commits mailing listllvm-commits at lists.llvm.orghttps://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/20210312/36dace95/attachment-0001.html>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4002 bytes
Desc: S/MIME Cryptographic Signature
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20210312/36dace95/attachment-0001.bin>
More information about the llvm-commits
mailing list