<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 3/12/21 5:32 PM, Mehdi AMINI wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CANF-O=bfNwUVjPoZYzNsCdwrQTcstTasKrK-F2qociTup6Gauw@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<div dir="ltr">
<div dir="ltr"><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Mar 12, 2021 at 4:08
PM Philip Reames <<a
href="mailto:listmail@philipreames.com"
moz-do-not-send="true">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p><br>
</p>
<div>On 3/12/21 3:55 PM, Mehdi AMINI wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div>Hey Philip,</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Fri, Mar 12,
2021 at 3:22 PM Philip Reames via llvm-commits
<<a href="mailto:llvm-commits@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-commits@lists.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px
0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p><br>
</p>
<div>On 3/12/21 2:59 PM, Jordan Rupprecht
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr"><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On
Fri, Mar 12, 2021 at 4:29 PM Philip
Reames <<a
href="mailto:listmail@philipreames.com"
target="_blank"
moz-do-not-send="true">listmail@philipreames.com</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p>Jordan,</p>
<p>Please revert this change.</p>
<p>There's a couple of problems
here:</p>
<ul>
<li>The change reverted looks
obviously innocent. (e.g. it's
bailing out of a transform
slightly more often)</li>
</ul>
</div>
</blockquote>
<div>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.</div>
</div>
</div>
</blockquote>
<p>You're correct, but missing the point I was
getting at. Admittedly, poorly worded. <br>
</p>
<p>While looking obviously innocent isn't
reason not to revert itself, it's definitely
reason to take a slightly closer look and
make sure there's nothing else going on.
This is particularly relevant given the
other points in this discussion.<br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<ul>
<li>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.</li>
</ul>
</div>
</blockquote>
<div>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.</div>
</div>
</div>
</blockquote>
<p>Bug? Discussion? Response to original
commit?</p>
<p>Your observation may be true *for you*. It
is not necessarily true for anyone else, and
you bear the burden of making the case.
Particularly when reverting a functional
fix.<br>
</p>
</div>
</blockquote>
<div><br>
</div>
<div>Sure, the author of the revert bears the
burden of providing everything needed for the
reproduction, but that does not mean we
shouldn't revert first and talk it through when
a problem is detected. I've been frequently
reverting patches when there were obvious
regressions and saying "I'm still working on
reducing the test case" (which frequently
required making it into something that won't
leak proprietary data...).</div>
</div>
</div>
</div>
</blockquote>
<br>
<p>So, let's start with the acknowledgement that all of
this is complicated. There's no absolutes here.</p>
<p>Having said that, given the whole of the circumstances,
I do not feel that the implied burden of proof was met
for this revert, in this specific case. As has been
acknowledged, the revert was rushed (i.e. no public
mention of a problem before revert after a change had
been in tree for days). If that hadn't been the case,
the implied bar would be much lower. <br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>It also seems to me that providing a clang
test case that repro at head is enough, I have
no problem generating IR if the author is asking
me to, but I don't consider this like a
prerequisite to revert either: clang is in-tree
and like every project in-tree we shouldn't
regress it unknowingly and/or without
coordination.</div>
</div>
</div>
</div>
</blockquote>
<p>Again, whole of circumstances. For a rushed revert,
days after submission, with no prior discussion, burden
is on the reverter. If this had been a hour or two
after commit, I'd have no problem with a C/C++ example,
or a link to a build bot. That was not the case here.<br>
</p>
<p>On the in tree point you make, I will not agree that
"just because something is in tree" there's no burden of
reduction for a case like this. Take your paragraph and
replace "clang" with "flang", or "mlir" or "gn build",
do you still feel the same way?</p>
</div>
</blockquote>
<div>"gn build" is explicitly not something supported, so no,
but otherwise for something like "flang" and "mlir" then yes
I feel the same way: we can't break these bots with patches
in the monorepo.</div>
<div><br>
</div>
<div>Now, of course we have a history in the community of
working together: if someone reverts your patch because you
broke Flang tests, this person may not be better suited than
you to reduce it, they may just be a buildcop in charge of
keeping the buildbot green.</div>
<div>The way I tend to see us proceeding in such cases is to
loop-in the Flang developers who bear the responsibility to
promptly help the LLVM commit author to get the reproducer
in a state that you can move forward with your patch.</div>
<div><br>
</div>
<div>Even staying within LLVM itself, breaking a bot that runs
test in Webassembly or PowerPC may be tricky for the LLVM
contributor to reproduce and they may need help from the
Webassembly/PowerPC/... folks who have access to the
hardware (or runtime environment) to test your patch and
reduce/debug further. The person who reverts is frequently
"just the messenger" and they are doing a community service
in putting back the tree in "good shape": it isn't rewarding
to debug and bisect, it isn't pleasant to revert changes,
yet this distributed testing / validation is also somehow
"free QA" for your commit.</div>
<div><br>
</div>
<div>In any case, I strongly believe that we should revert
first and debug collectively: first because it is always
very easy to re-land a patch and keep bots green is a
priority, but also because the person who reverts may not be
in the best position to provide the repro themselves, and
that seems fine to me.</div>
<div><br>
</div>
<div>There are also environments where tests can take a long
time to run, and bisecting isn't trivial (the blame-list can
be large). I am fairly sure I sometimes took a few days
bisecting and isolating a single faulty LLVM commit when I
was at Apple (think about integrating LLVM into Swift in
bulk every other day, run a bunch of public and internal
tests, sometimes on embedded devices). So a few hours vs a
few days isn't a strong discriminant to me, a few months may
be another story, but it seems we all have a different
sensitivity here.</div>
</div>
</div>
</blockquote>
<p>Mehdi, in generalities, I see all of the points you're making
about reverts and agree with them. The thing which changes my
interpretation in this particular situation is that the change
reverted was itself a functional fix. Reverting didn't put us
back into a green state, it put us into a differently broken
state. That might have been reasonable, or it might not have.
Without context - which the reverting commit didn't include - it's
impossible to tell which is true. My default assumption was that
the balance hadn't been considered, and thus the start to this
whole thread. <br>
</p>
<p>Does that make sense to you? I don't think we're actually in
disagreement here.<br>
</p>
<blockquote type="cite"
cite="mid:CANF-O=bfNwUVjPoZYzNsCdwrQTcstTasKrK-F2qociTup6Gauw@mail.gmail.com">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p>I'll warn you, this is a hill I'm willing to die on.
:) It both affects my personal workflows immensely, and
is also something I see as being really important to
community as a whole. I will say that if you want to
discuss this in abstract (not specific to the particular
revert in question), we should probably move this to an
llvm-dev thread.<br>
</p>
<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px
0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p> </p>
<p>To be clear, I'm not saying you're wrong.
I'm saying you haven't given anyone else
enough information to tell if you are or
not. <br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<ul>
<li>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.</li>
</ul>
</div>
</blockquote>
<div>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. <br>
</div>
</div>
</div>
</blockquote>
Thanks for the acknowledgement. To be fair,
it would have been less of an issue if the
reproducer worked straight forwardly.<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<ul>
<li>Failure to provide an IR test
case.</li>
</ul>
</div>
</blockquote>
<div>(ditto, but see one below) </div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<ul>
<li>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.</li>
</ul>
</div>
</blockquote>
<div>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.</div>
</div>
</div>
</blockquote>
<p>I actually don't think this is going off
track at all. Our default is for IR test
cases for IR problems. Any reverting commit
should generally include a test case
suitable for checkin (w/a bit of cleanup)
once the patch is fixed. I'll admit, we're
not strict about this, but the expectation
is definitely there.<br>
</p>
</div>
</blockquote>
<div>I wasn't aware of this expectation, in
general I expect a reproducer that reproduces
*in-tree*. </div>
<div>Just last week I reverted a case where it
broke the bootstrap of clang, I am not working
on clang but I have a bot that bootstrap clang
and then I use this clang to test my code. I
consider myself doing a community service by
reverting fast and providing reproduction
instructions to the author.</div>
<div>However I believe that the burden of
debugging this will be on the patch author, even
though the author is purely changing an LLVM IR
pass. I wouldn't go and debug stage 2 and
minimize and IR reproducer from the clang
pipeline when many bots are broken.</div>
</div>
</div>
</div>
</blockquote>
See my point above about whole of circumstances and
timeframes.<br>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px
0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p> </p>
<p>The discussion of the hybrid configuration
is relevant *because* you didn't provide a
test case in a form I could easily use. I
don't work on clang, don't build it
regularly, and shouldn't have to investigate
a LLVM codegen regression.</p>
</div>
</blockquote>
<div>I am not sure we have a wide agreement here:
as an author you may ask for help to get a
smaller repro, but my take is that if you break
clang you may have to build it yourself. In the
example above, I provided the cmake invocation
that would make it crash in stage2. This is
hermetic in the monorepo, does not require any
external dependency, I hope it passes the bar
for revert and reproducer.</div>
<div><br>
</div>
<div>Maybe we should have a larger discussion
about this on llvm-dev@ and document this all?
Apparently we have different implicit
assumptions here, because over the years it
seemed normal to me to receive C++ input example
when I broke clang with my LLVM changes</div>
</div>
</div>
</div>
</blockquote>
<p>See above, but yes, some broader discussion may be
warranted. I don't think my take is out of line with
historical practice, but it may be time to document that
if we're getting disagreement. <br>
</p>
<p>If you start the thread, please try to reflect the
complexities and the differences different timeframes
bring into discussion.<br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>Cheers,</div>
<div><br>
</div>
<div>-- </div>
<div>Mehdi</div>
<blockquote class="gmail_quote" style="margin:0px
0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p> <br>
</p>
<p>(See also below.)<br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<ul>
</ul>
<p>Philip<br>
</p>
<p>p.s. To be clear, I'm happy to
look at your original issue this
afternoon if you've got something
I can reproduce.</p>
</div>
</blockquote>
<div>Here's an IR reproducer with IR
generated from ToT before my revert
(at
dfd27ebbd0eb137c9a439b7c537bb87ba903efd3):</div>
</div>
</div>
</blockquote>
<p>Ok, we have a problem here. None of the
following reproduce for me:<br>
</p>
<p>$ ./llc -O2 jordan.ll<br>
$ ./llc -O2 jordan.ll<br>
$ ./llc -O2 jordan.ll --filetype=obj<br>
$ ./llc -O3 jordan.ll --filetype=obj<br>
</p>
<p>LLC is ToT, just built with your change
reverted locally.<br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div>$ bin/clang -c /tmp/repro.cc -O1 -S
-emit-llvm -o /tmp/repro.ll</div>
<div>$ bin/clang -c /tmp/repro.ll -O2 -o
/tmp/repro.o # hangs</div>
</div>
</div>
</blockquote>
<p>Given the preceding, I am now asserting
this is likely some clang specific problem.
I've got a build of clang running now, will
report back in a bit.</p>
<p>The other option is that this is someway
specific to your configuration. <br>
</p>
<p><br>
</p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div><br>
</div>
<div>; ModuleID = '/tmp/repro.cc'<br>
source_filename = "/tmp/repro.cc"<br>
target datalayout =
"e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"<br>
target triple =
"x86_64-unknown-linux-gnu"<br>
<br>
%class.D = type { i64 }<br>
%class.a = type { %class.g }<br>
%class.g = type { i32*, i32* }<br>
<br>
$_ZNK1aIliEixEl = comdat any<br>
<br>
$_ZNK1aIliE1jEv = comdat any<br>
<br>
@o = dso_local local_unnamed_addr
global i32 0, align 4<br>
@p = dso_local local_unnamed_addr
global i32 0, align 4<br>
@.str = private unnamed_addr constant
[1 x i8] zeroinitializer, align 1<br>
<br>
; Function Attrs: uwtable mustprogress<br>
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 {<br>
%3 = call i64
@_ZNK1aIliEixEl(%class.a* nonnull
dereferenceable(16) %1, i64 0) #3<br>
%4 = icmp eq i64 %3, 0<br>
br i1 %4, label %26, label %5<br>
<br>
5:
; preds = %2<br>
%6 = call i64
@_ZNK1aIliE1jEv(%class.a* nonnull
dereferenceable(16) %1)<br>
%7 = icmp eq i64 %6, 0<br>
br i1 %7, label %26, label %8<br>
<br>
8:
; preds = %5<br>
%9 = getelementptr inbounds
%class.D, %class.D* %0, i64 0, i32 0<br>
br label %10<br>
<br>
10:
; preds = %8, %18<br>
%11 = phi i64 [ 0, %8 ], [ %23, %18
]<br>
%12 = call i64
@_ZNK1aIliEixEl(%class.a* nonnull
dereferenceable(16) %1, i64 %11) #3<br>
%13 = icmp eq i64 %12, 0<br>
br i1 %13, label %18, label %14<br>
<br>
14:
; preds = %10, %14<br>
%15 = load i32, i32* @o, align 4,
!tbaa !2<br>
%16 = call i32
@_Z3fn1IiiEiT_T0_PKc(i32 %15, i32 0,
i8* getelementptr inbounds ([1 x i8],
[1 x i8]* @.str, i64 0, i64 0))<br>
%17 = icmp eq i32 %16, 0<br>
br i1 %17, label %18, label %14,
!llvm.loop !6<br>
<br>
18:
; preds = %14, %10<br>
%19 = call i64
@_ZNK1aIliEixEl(%class.a* nonnull
dereferenceable(16) %1, i64 %11) #3<br>
%20 = load i32, i32* @p, align 4,
!tbaa !2<br>
%21 = sext i32 %20 to i64<br>
%22 = sdiv i64 %19, %21<br>
store i64 %22, i64* %9, align 8,
!tbaa !9<br>
%23 = add i64 %11, 1<br>
%24 = call i64
@_ZNK1aIliE1jEv(%class.a* nonnull
dereferenceable(16) %1)<br>
%25 = icmp eq i64 %24, 0<br>
br i1 %25, label %26, label %10,
!llvm.loop !12<br>
<br>
26:
; preds = %18, %5, %2<br>
ret void<br>
}<br>
<br>
; Function Attrs: nounwind uwtable
willreturn mustprogress<br>
define linkonce_odr dso_local i64
@_ZNK1aIliEixEl(%class.a* nonnull
dereferenceable(16) %0, i64 %1)
local_unnamed_addr #1 comdat align 2 {<br>
%3 = getelementptr inbounds
%class.a, %class.a* %0, i64 0, i32 0,
i32 0<br>
%4 = load i32*, i32** %3, align 8,
!tbaa !13<br>
%5 = getelementptr inbounds i32,
i32* %4, i64 %1<br>
%6 = load i32, i32* %5, align 4,
!tbaa !2<br>
%7 = sext i32 %6 to i64<br>
ret i64 %7<br>
}<br>
<br>
; Function Attrs: nounwind uwtable
willreturn mustprogress<br>
define linkonce_odr dso_local i64
@_ZNK1aIliE1jEv(%class.a* nonnull
dereferenceable(16) %0)
local_unnamed_addr #1 comdat align 2 {<br>
%2 = getelementptr inbounds
%class.a, %class.a* %0, i64 0, i32 0,
i32 1<br>
%3 = load i32*, i32** %2, align 8,
!tbaa !16<br>
%4 = getelementptr inbounds
%class.a, %class.a* %0, i64 0, i32 0,
i32 0<br>
%5 = load i32*, i32** %4, align 8,
!tbaa !13<br>
%6 = ptrtoint i32* %3 to i64<br>
%7 = ptrtoint i32* %5 to i64<br>
%8 = sub i64 %6, %7<br>
%9 = ashr exact i64 %8, 2<br>
ret i64 %9<br>
}<br>
<br>
declare dso_local i32
@_Z3fn1IiiEiT_T0_PKc(i32, i32, i8*)
local_unnamed_addr #2<br>
<br>
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" }<br>
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" }<br>
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" }<br>
attributes #3 = { nounwind }<br>
<br>
!llvm.module.flags = !{!0}<br>
!llvm.ident = !{!1}<br>
<br>
!0 = !{i32 1, !"wchar_size", i32 4}<br>
!1 = !{!"clang version 13.0.0 (<a
href="https://github.com/llvm/llvm-project.git"
target="_blank"
moz-do-not-send="true">https://github.com/llvm/llvm-project.git</a>
dfd27ebbd0eb137c9a439b7c537bb87ba903efd3)"}<br>
!2 = !{!3, !3, i64 0}<br>
!3 = !{!"int", !4, i64 0}<br>
!4 = !{!"omnipotent char", !5, i64 0}<br>
!5 = !{!"Simple C++ TBAA"}<br>
!6 = distinct !{!6, !7, !8}<br>
!7 = !{!"llvm.loop.mustprogress"}<br>
!8 = !{!"llvm.loop.unroll.disable"}<br>
!9 = !{!10, !11, i64 0}<br>
!10 = !{!"_ZTS1D", !11, i64 0}<br>
!11 = !{!"long", !4, i64 0}<br>
!12 = distinct !{!12, !7, !8}<br>
!13 = !{!14, !15, i64 0}<br>
!14 = !{!"_ZTS1g", !15, i64 0, !15,
i64 8}<br>
!15 = !{!"any pointer", !4, i64 0}<br>
</div>
<div>!16 = !{!14, !15, i64 8}</div>
<div> </div>
<blockquote class="gmail_quote"
style="margin:0px 0px 0px
0.8ex;border-left-width:1px;border-left-style:solid;border-left-color:rgb(204,204,204);padding-left:1ex">
<div>
<p> </p>
<div>On 3/12/21 1:59 PM, Jordan
Rupprecht via llvm-commits wrote:<br>
</div>
<blockquote type="cite">
<pre>Author: Jordan Rupprecht
Date: 2021-03-12T13:59:14-08:00
New Revision: 8d20f2c2c66eb486ff23cc3d55a53bd840b36971
URL: <a href="https://github.com/llvm/llvm-project/commit/8d20f2c2c66eb486ff23cc3d55a53bd840b36971" target="_blank" moz-do-not-send="true">https://github.com/llvm/llvm-project/commit/8d20f2c2c66eb486ff23cc3d55a53bd840b36971</a>
DIFF: <a href="https://github.com/llvm/llvm-project/commit/8d20f2c2c66eb486ff23cc3d55a53bd840b36971.diff" target="_blank" moz-do-not-send="true">https://github.com/llvm/llvm-project/commit/8d20f2c2c66eb486ff23cc3d55a53bd840b36971.diff</a>
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"
-
-@b = dso_local local_unnamed_addr global i64 0, align 8
-@c = dso_local local_unnamed_addr global i64 0, align 8
-@d = dso_local local_unnamed_addr global i64 0, align 8
-@e = dso_local local_unnamed_addr global i64 0, align 8
-@f = dso_local local_unnamed_addr global i64 0, align 8
-@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 list
<a href="mailto:llvm-commits@lists.llvm.org" target="_blank" moz-do-not-send="true">llvm-commits@lists.llvm.org</a>
<a href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" target="_blank" moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a>
</pre>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
</div>
_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@lists.llvm.org"
target="_blank" moz-do-not-send="true">llvm-commits@lists.llvm.org</a><br>
<a
href="https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits"
rel="noreferrer" target="_blank"
moz-do-not-send="true">https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</blockquote>
</div>
</div>
</div>
</blockquote>
</div>
</blockquote>
</div>
</div>
</blockquote>
</body>
</html>