[llvm] r295539 - AMDGPU/R600: Assert on infinite loop in EmitClauseMarkers

[David Blaikie via llvm-commits]

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)


>
> >
> > * 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 --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20170221/5a72ba8f/attachment.html>