On Wed, Nov 9, 2016 at 10:35 AM, Samuel Pitoiset <[email protected]> wrote: > > > On 11/09/2016 04:19 PM, Ilia Mirkin wrote: >> >> On Wed, Nov 9, 2016 at 10:10 AM, Samuel Pitoiset >> <[email protected]> wrote: >>> >>> >>> >>> On 11/09/2016 03:58 PM, Ilia Mirkin wrote: >>>> >>>> >>>> On Wed, Nov 9, 2016 at 9:20 AM, Samuel Pitoiset >>>> <[email protected]> wrote: >>>>> >>>>> >>>>> For all instructions with 3 sources (like OP_SLCT), src2 needs >>>>> to be destroyed because srcExists(2) will return true although >>>>> it's actually undefined. >>>>> >>>>> Spotted with my ADD3 series. >>>> >>>> >>>> >>>> Sounds like the ADD3 series' addition to this function doesn't handle >>>> this scenario. It's already properly handled by FMA/MAD, for example. >>> >>> >>> >>> Nope, it's correctly handled in expr() in my series. >>> >>> In ConstantFolding::visit(BasicBlock *bb), I added a new opnd2() method >>> which is used for folding ADD3(a,b,c)-> ADD3(b,a+c) or ->ADD3(a,b+c) >>> because >>> the pass doesn't currently implement such a case. >>> >>> Though, opnd2() is called if srcExists(2) is true and if src0/src1 or >>> src0/src2 are immediate values. But as this is done after the opnd3() >>> call, >>> if the instruction has been constant folded (just before) but src2 not >>> erased, it will try to get the immediates and crash... which is expected. >>> >>> This just makes expr() more robust against that, and it's not entirely >>> related to my series, because if you try to detect more scenarios where >>> immediates can be folded *after* opnd3() you will hit the issue for sure. >>> >>> src1 is already erased in expr(), I don't see why src2 should not be as >>> well >>> (when optimizing to SAT/MOV). >> >> >> src2 is also erased. >> >> switch (i->op) { >> case OP_MAD: >> case OP_FMA: { >> ... >> i->setSrc(2, NULL); >> } >> >> You need to add a case for ADD3 here that does the same thing. And >> also immediate re-executes expr/opnd in case the other arg was *also* >> an immed. That should avoid taking up an extra fold iteration on it. >> > > I know, I already added all the dance there. Let me explain again. :-) > > I found the crash with OP_SLCT which has 3 sources. expr() has been called, > and that SLCT has been optimized to MOV because src0 and src1 were 0. Thus, > srcExists(1) will return false while srcExists(2) will return true because > it has not been erased. right? > > The thing is, after the opnd3() call in the visit func, I added some logic > in order to detect if src0/src2 or src1/src2 are immediates.
Then it's a bug in the SLCT handling. Look - setting src(2) to null like that is a bug. It might be a predicate source (for ->setPredicate), or a couple other things. If you're changing ops from one to another you should be using moveSources which will adjust everything as well. Although changing from SLCT to a MOV should be impossible -- IIRC SLCT is a CmpInstruction while MOV should be a regular one. -ilia > > Have a look at this chunk http://hastebin.com/ihalexobus.cpp . > > >>> >>> >>>> >>>>> >>>>> Signed-off-by: Samuel Pitoiset <[email protected]> >>>>> --- >>>>> src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp | 1 + >>>>> 1 file changed, 1 insertion(+) >>>>> >>>>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp >>>>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp >>>>> index 9cf6ddc..ef31436 100644 >>>>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp >>>>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_peephole.cpp >>>>> @@ -772,6 +772,7 @@ ConstantFolding::expr(Instruction *i, >>>>> break; >>>>> default: >>>>> i->op = i->saturate ? OP_SAT : OP_MOV; /* SAT handled by unary() >>>>> */ >>>>> + i->setSrc(2, NULL); >>>>> break; >>>>> } >>>>> i->subOp = 0; >>>>> -- >>>>> 2.10.2 >>>>> >>>>> _______________________________________________ >>>>> mesa-dev mailing list >>>>> [email protected] >>>>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev >>> >>> >>> >>> -- >>> -Samuel > > > -- > -Samuel _______________________________________________ mesa-dev mailing list [email protected] https://lists.freedesktop.org/mailman/listinfo/mesa-dev
