[Mesa-dev] [PATCH v2] nvc0/ir: return 0 in imageLoad on incomplete textures

[Karol Herbst]

On Sat, Jul 14, 2018 at 8:44 PM, Ilia Mirkin <imirkin at alum.mit.edu> wrote:
> On Fri, Jun 29, 2018 at 10:27 PM, Karol Herbst <kherbst at redhat.com> wrote:
>> We already guarded all OP_SULDP against out of bound accesses, but those
>> ended up just reusing whatever value was stored in the dest registers.
>>
>> fixes CTS test shader_image_load_store.incomplete_textures
>>
>> v2: fix for loads not ending up with predicates (bindless_texture)
>>
>> Signed-off-by: Karol Herbst <kherbst at redhat.com>
>> ---
>>  .../nouveau/codegen/nv50_ir_lowering_nvc0.cpp | 33 +++++++++++++++++--
>>  .../nouveau/codegen/nv50_ir_lowering_nvc0.h   |  1 +
>>  2 files changed, 31 insertions(+), 3 deletions(-)
>>
>> 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 5723847234e..f55e9a34c59 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.cpp
>> @@ -2180,13 +2180,36 @@ NVC0LoweringPass::convertSurfaceFormat(TexInstruction *su)
>>     }
>>  }
>>
>> +void
>> +NVC0LoweringPass::insertOOBSurfaceOpResult(TexInstruction *su)
>> +{
>> +   if (!su->getPredicate())
>> +      return;
>> +
>> +   bld.setPosition(su, true);
>> +
>> +   for (unsigned i = 0; su->defExists(i); ++i) {
>> +      ValueDef &def = su->def(i);
>> +
>> +      Instruction *mov = bld.mkMov(bld.getSSA(), bld.loadImm(NULL, 0));
>> +      assert(su->cc == CC_NOT_P);
>> +      mov->setPredicate(CC_P, su->getPredicate());
>> +      Instruction *uni = bld.mkOp2(OP_UNION, TYPE_U32, bld.getSSA(), NULL, mov->getDef(0));
>> +
>> +      def.replace(uni->getDef(0), true);
>> +      uni->setSrc(0, def.get());
>
> Pretty sure this doesn't work. You do def.replace(x, true) which will
> set the def's value to x. And then you do def.get() which will just
> return x -- not what you want. You're going to end up with a union
> that has its def as an argument.
>
> It might all end up panning out sometimes, but check the IR that's
> generated. Perhaps I'm misreading what def.replace does.
>

right.. I have a local patch to fix this with a
"def.replace(uni->getDef(0), false);". I simply forgot I had this fix
locally.

>> +   }
>> +}
>> +
>>  void
>>  NVC0LoweringPass::handleSurfaceOpNVE4(TexInstruction *su)
>>  {
>>     processSurfaceCoordsNVE4(su);
>>
>> -   if (su->op == OP_SULDP)
>> +   if (su->op == OP_SULDP) {
>>        convertSurfaceFormat(su);
>> +      insertOOBSurfaceOpResult(su);
>> +   }
>>
>>     if (su->op == OP_SUREDB || su->op == OP_SUREDP) {
>>        assert(su->getPredicate());
>> @@ -2296,8 +2319,10 @@ NVC0LoweringPass::handleSurfaceOpNVC0(TexInstruction *su)
>>
>>     processSurfaceCoordsNVC0(su);
>>
>> -   if (su->op == OP_SULDP)
>> +   if (su->op == OP_SULDP) {
>>        convertSurfaceFormat(su);
>> +      insertOOBSurfaceOpResult(su);
>> +   }
>>
>>     if (su->op == OP_SUREDB || su->op == OP_SUREDP) {
>>        const int dim = su->tex.target.getDim();
>> @@ -2397,8 +2422,10 @@ NVC0LoweringPass::handleSurfaceOpGM107(TexInstruction *su)
>>  {
>>     processSurfaceCoordsGM107(su);
>>
>> -   if (su->op == OP_SULDP)
>> +   if (su->op == OP_SULDP) {
>>        convertSurfaceFormat(su);
>> +      insertOOBSurfaceOpResult(su);
>> +   }
>>
>>     if (su->op == OP_SUREDP) {
>>        Value *def = su->getDef(0);
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
>> index 91771fbf7e9..d7350d03b78 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_lowering_nvc0.h
>> @@ -143,6 +143,7 @@ private:
>>     void processSurfaceCoordsNVE4(TexInstruction *);
>>     void processSurfaceCoordsNVC0(TexInstruction *);
>>     void convertSurfaceFormat(TexInstruction *);
>> +   void insertOOBSurfaceOpResult(TexInstruction *);
>>     Value *calculateSampleOffset(Value *sampleID);
>>
>>  protected:
>> --
>> 2.17.1
>>
>> _______________________________________________
>> mesa-dev mailing list
>> mesa-dev at lists.freedesktop.org
>> https://lists.freedesktop.org/mailman/listinfo/mesa-dev