[Mesa-dev] [RFC 20/24] nvc0/ir: prevent out of bounds when no images are bound

[Samuel Pitoiset]

On 04/13/2016 10:44 PM, Ilia Mirkin wrote:
> On Tue, Apr 12, 2016 at 7:57 PM, Samuel Pitoiset
> <samuel.pitoiset at gmail.com> wrote:
>> Checking if the image address is not 0 should be enough to prevent
>> read faults. To improve robustness, make sure that the destination
>> value of atomic operations is correctly initialized in case the
>> instruction is not performed.
>>
>> Signed-off-by: Samuel Pitoiset <samuel.pitoiset at gmail.com>
>> ---
>>   .../drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp | 19 ++++++++++++++++++-
>>   1 file changed, 18 insertions(+), 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 bd7e4bd..253084e 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
>> @@ -1774,6 +1774,13 @@ NVC0LoweringPass::processSurfaceCoordsNVE4(TexInstruction *su)
>>      su->setSrc(0, addr);
>>      su->setSrc(1, v);
>>      su->setSrc(2, pred);
>> +
>> +   // prevent read fault when the image is not actually bound
>> +   CmpInstruction *pred1 =
>> +      bld.mkCmp(OP_SET, CC_EQ, TYPE_U32, bld.getSSA(1, FILE_PREDICATE),
>> +                TYPE_U32, bld.mkImm(0),
>> +                loadSuInfo32(ind, base + NVE4_SU_INFO_ADDR));
>> +   su->setPredicate(CC_NOT_P, pred1->getDef(0));
>>   }
>>
>>   static DataType
>> @@ -1893,7 +1900,6 @@ NVC0LoweringPass::handleSurfaceOpNVE4(TexInstruction *su)
>>         convertSurfaceFormat(su);
>>
>>      if (su->op == OP_SUREDB || su->op == OP_SUREDP) {
>> -      // FIXME: for out of bounds access, destination value will be undefined !
>>         Value *pred = su->getSrc(2);
>>         CondCode cc = CC_NOT_P;
>>         if (su->getPredicate()) {
>> @@ -1915,7 +1921,18 @@ NVC0LoweringPass::handleSurfaceOpNVE4(TexInstruction *su)
>>         if (su->subOp == NV50_IR_SUBOP_ATOM_CAS)
>>            red->setSrc(2, su->getSrc(4));
>>         red->setIndirect(0, 0, su->getSrc(0));
>> +
>> +      // make sure to initialize dst value when the atomic operation is not
>> +      // performed
>> +      Instruction *mov = bld.mkMov(bld.getScratch(), bld.loadImm(NULL, 0));
>
> I tend to prefer bld.getSSA(). It's shorter.
>
>> +
>> +      assert(cc == CC_NOT_P);
>>         red->setPredicate(cc, pred);
>> +      mov->setPredicate(CC_P, pred);
>> +
>> +      bld.mkOp2(OP_UNION, TYPE_U32, su->getDef(0),
>> +                red->getDef(0), mov->getDef(0));
>
> This will only work if red->getDef() != su->getDef()... make sure that
> red gets a fresh def (bld.getSSA())

This should not happen, but I will check.

>
> With that verified,
>
> Reviewed-by: Ilia Mirkin <imirkin at alum.mit.edu>
>
>> +
>>         delete_Instruction(bld.getProgram(), su);
>>         handleCasExch(red, true);
>>      }
>> --
>> 2.8.0
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev