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 :/)

Greetings,
Tobias



        // check if we are moving an immediate, propagate it in that case
        if (!ld || ld->fixed || (ld->op != OP_LOAD && ld->op != OP_MOV) ||
              !(ld->src(0).getFile() == FILE_IMMEDIATE))
           bld.mkMovToReg(s, i->getSrc(s));
        else {
+         assert(ld->getSrc(0) != NULL);
           bld.mkMovToReg(s, ld->getSrc(0));
           // Clear the src, to make code elimination possible here before we
           // delete the instruction i later
--
2.21.0

_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to