[llvm] r295539 - AMDGPU/R600: Assert on infinite loop in EmitClauseMarkers
Jan Vesely via llvm-commits
llvm-commits at lists.llvm.org
Wed Feb 22 07:24:38 PST 2017
On Tue, 2017-02-21 at 05:02 +0000, David Blaikie wrote:
> On Mon, Feb 20, 2017 at 9:46 AM Jan Vesely <jan.vesely at rutgers.edu> wrote:
>
> > On Sat, 2017-02-18 at 20:44 +0000, David Blaikie wrote:
> > > This seems not quite right for a few reasons:
> > >
> > > * If this code is reachable, an assertion isn't appropriate (assertions
> > > should not be reachable - there are some tests for assertions failures in
> > > unit tests, to demonstrate that those APIs assert fail on misuse, for
> > > example) - maybe report_fatal_error is what's required here?
> >
> > the asserted condition should not be reachable. It's a bug that needs
> > fixing.
> >
>
> FWIW, it's not terribly common to commit tests for bugs before they're
> fixed. If you're already planning on doing the work - or even have a bug
> tracking it, it's probably better to put the test case on the bug. (yeah,
> there are some cases where this isn't ideal - if there's some related
> testing nearby it can be handy to verify sub-optimal transforms, etc, and
> write FIXME nearby, so that when a bug is fixed the fixer can easily find
> an existing test case/related test cases to group the coverage under, and
> may reduce the work needed, etc)
thanks for the insight. The original patch did not include a test, it
was added per reviewer's request. I wanted to add the assertion
separately because there is more than one way to hit it.
anyway, the underlying bug is fixed and the test enabled in D30230.
regards,
Jan
>
>
> >
> > >
> > > * XFAIL isn't the right tool for a test for failure - "not llc" or the
> >
> > like
> > > (I'm not sure if lit supports that exact syntax, or it requires something
> > > else) would probably be the right thing.
> >
> > XFAIL is temporary, fix of the underlying bug will enable the test.
> >
> > >
> > > * This test would (if it remains in tree) need "REQUIRES: Asserts"
> > > otherwise it introduces an infinite loop in non-asserts builds. (I'm not
> > > sure why this isn't breaking any release building bot with the AMDGPU
> > > backend enabled)
> >
> > thank for noticing.
> > it was addressed by nakamura takumi in r295591.
> >
> >
> > regards,
> > Jan
> >
> > >
> > > On Fri, Feb 17, 2017 at 8:35 PM Jan Vesely via llvm-commits <
> > > llvm-commits at lists.llvm.org> wrote:
> > >
> > > > Author: jvesely
> > > > Date: Fri Feb 17 22:24:10 2017
> > > > New Revision: 295539
> > > >
> > > > URL: http://llvm.org/viewvc/llvm-project?rev=295539&view=rev
> > > > Log:
> > > > AMDGPU/R600: Assert on infinite loop in EmitClauseMarkers
> > > >
> > > > Differential Revision: https://reviews.llvm.org/D29792
> > > >
> > > > Added:
> > > > llvm/trunk/test/CodeGen/AMDGPU/r600.alu-limits.ll
> > > > Modified:
> > > > llvm/trunk/lib/Target/AMDGPU/R600EmitClauseMarkers.cpp
> > > >
> > > > Modified: llvm/trunk/lib/Target/AMDGPU/R600EmitClauseMarkers.cpp
> > > > URL:
> > > >
> >
> > http://llvm.org/viewvc/llvm-project/llvm/trunk/lib/Target/AMDGPU/R600EmitClauseMarkers.cpp?rev=295539&r1=295538&r2=295539&view=diff
> > > >
> > > >
> >
> > ==============================================================================
> > > > --- llvm/trunk/lib/Target/AMDGPU/R600EmitClauseMarkers.cpp (original)
> > > > +++ llvm/trunk/lib/Target/AMDGPU/R600EmitClauseMarkers.cpp Fri Feb 17
> > > > 22:24:10 2017
> > > > @@ -321,9 +321,11 @@ public:
> > > > if (I != MBB.end() && I->getOpcode() == AMDGPU::CF_ALU)
> > > > continue; // BB was already parsed
> > > > for (MachineBasicBlock::iterator E = MBB.end(); I != E;) {
> > > > - if (isALU(*I))
> > > > - I = MakeALUClause(MBB, I);
> > > > - else
> > > > + if (isALU(*I)) {
> > > > + auto next = MakeALUClause(MBB, I);
> > > > + assert(next != I);
> > > > + I = next;
> > > > + } else
> > > > ++I;
> > > > }
> > > > }
> > > >
> > > > Added: llvm/trunk/test/CodeGen/AMDGPU/r600.alu-limits.ll
> > > > URL:
> > > >
> >
> > http://llvm.org/viewvc/llvm-project/llvm/trunk/test/CodeGen/AMDGPU/r600.alu-limits.ll?rev=295539&view=auto
> > > >
> > > >
> >
> > ==============================================================================
> > > > --- llvm/trunk/test/CodeGen/AMDGPU/r600.alu-limits.ll (added)
> > > > +++ llvm/trunk/test/CodeGen/AMDGPU/r600.alu-limits.ll Fri Feb 17
> >
> > 22:24:10
> > > > 2017
> > > > @@ -0,0 +1,28 @@
> > > > +; RUN: opt -loop-unroll -unroll-threshold=2000 -S < %s | llc
> >
> > -march=r600
> > > > -mcpu=cypress | FileCheck %s
> > > > +; XFAIL: *
> > > > +
> > > > +; CHECK: {{^}}@alu_limits:
> > > > +
> > > > +%struct.foo = type {i32, i32, i32}
> > > > +
> > > > +define void @alu_limits(i32 addrspace(1)* %out, %struct.foo* %in, i32
> > > > %offset) {
> > > > +entry:
> > > > + %ptr = getelementptr inbounds %struct.foo, %struct.foo* %in, i32 1,
> >
> > i32
> > > > 2
> > > > + %x = load i32, i32 *%ptr, align 4
> > > > + br label %loop
> > > > +loop:
> > > > + %i = phi i32 [ 100, %entry ], [ %nexti, %loop ]
> > > > + %val = phi i32 [ 1, %entry ], [ %nextval, %loop ]
> > > > +
> > > > + %nexti = sub i32 %i, 1
> > > > +
> > > > + %y = xor i32 %x, %i
> > > > + %nextval = mul i32 %val, %y
> > > > +
> > > > + %cond = icmp ne i32 %nexti, 0
> > > > + br i1 %cond, label %loop, label %end
> > > > +end:
> > > > + %out_val = add i32 %nextval, 4
> > > > + store i32 %out_val, i32 addrspace(1)* %out, align 4
> > > > + ret void
> > > > +}
> > > >
> > > >
> > > > _______________________________________________
> > > > llvm-commits mailing list
> > > > llvm-commits at lists.llvm.org
> > > > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits
> > > >
-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 819 bytes
Desc: This is a digitally signed message part
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170222/550dba39/attachment.sig>
More information about the llvm-commits
mailing list