On Fri, Jul 19, 2019 at 12:07 PM Tobias Klausmann <tobias.johannes.klausm...@mni.thm.de> wrote: > On 19.07.19 15:39, Eric Engestrom wrote: > > On Friday, 2019-07-19 13:56:30 +0200, Mark Menzynski wrote: > >> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=111007 > >> Fixes: https://bugs.freedesktop.org/show_bug.cgi?id=111167 > > `Fixes:` is used to indicate the commit that introduced the code being > > fixed, such as: > > Fixes: 1c4e6d7ca83578caf521 ("nvc0/ir: propagate immediates to CALL > > input MOVs") > > This tag is used by our tools to backport fixes to the relevant stable > > releases. > > > > Bugzilla entries are referenced using the `Bugzilla:` prefix. > > > >> Signed-off-by: Mark Menzynski <mmenz...@redhat.com> > >> --- > >> src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp | 2 +- > >> 1 file changed, 1 insertion(+), 1 deletion(-) > >> > >> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp > >> b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp > >> index aca3b0afb1e..1f702a987d8 100644 > >> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp > >> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp > >> @@ -51,12 +51,12 @@ NVC0LegalizeSSA::handleDIV(Instruction *i) > >> // Generate movs to the input regs for the call we want to generate > >> for (int s = 0; i->srcExists(s); ++s) { > >> Instruction *ld = i->getSrc(s)->getInsn(); > >> - assert(ld->getSrc(0) != NULL); > > I'll admit I don't know anything about this code, but it looks like > > this might be a better fix? > > assert(ld == NULL || ld->getSrc(0) != NULL) > > > > I cc'ed Tobias who wrote this code as he'll probably know best. > > Thanks for letting me know, but i'm kind of out of the loop and sadly > don't remember too much about this one. > > Yet while having a look at the code i somehow wonder if there is a > possibility to have > > if (!ld || ld->fixed || (ld->op != OP_LOAD && ld->op != OP_MOV) || > !(ld->src(0).getFile() == FILE_IMMEDIATE)) > > crash at ld->src(0), as this is only set when a value is added to the > instruction. So in the case ld->src(0) is legal, ld->getSrc(0) should be as > well and we would need the assert at the beginning... > > PS: Did a functional change bring this to the light? (I ran piglit in front > of every patch sumbission :/)
Nope. Just weird shaders that divide and/or mod 0 by a non-constant amount. There's an argument to just remove that assert entirely - not sure that it's adding a whole lot, except complication. _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/mesa-dev
