Nouveau - Mar 2016 - [PATCH mesa 6/6] nouveau: codegen: Disable more old resource handling code

On 03/16/2016 10:23 AM, Hans de Goede wrote:
> Commit c3083c7082 ("nv50/ir: add support for BUFFER accesses")
disabled /
> commented out some of the old resource handling code, but not all of it.
>
> Effectively all of it is dead already, if we ever enter the old code
> paths in handeLOAD / handleSTORE / handleATOM we will get an exception
> due to trying to access the now always zero-sized resources vector.
>
> Make non buffer / memory file accesses not being supported in these
> functions more explicit and comment out a whole bunch of dead code.
>
> Also remove the magic file-indexe defines from the old resource code
> from include/pipe/p_shader_tokens.h as those are no longer used now
> (which is a good thing).
>
> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
> ---
>   .../drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp  | 42
+++++++++++++++-------
>   src/gallium/include/pipe/p_shader_tokens.h         |  9 -----
>   2 files changed, 30 insertions(+), 21 deletions(-)
>
> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> index c167c4a..115d0bb 100644
> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
> @@ -856,12 +856,14 @@ public:
>      };
>      std::vector<TextureView> textureViews;
>
> +   /*
>      struct Resource {
>         uint8_t target; // TGSI_TEXTURE_*
>         bool raw;
>         uint8_t slot; // $surface index
>      };
>      std::vector<Resource> resources;
> +   */
>
>      struct MemoryFile {
>         uint8_t mem_type; // TGSI_MEMORY_TYPE_*
> @@ -1423,8 +1425,8 @@ private:
>      void handleLIT(Value *dst0[4]);
>      void handleUserClipPlanes();
>
> -   Symbol *getResourceBase(int r);
> -   void getResourceCoords(std::vector<Value *>&, int r, int s);
> +   // Symbol *getResourceBase(int r);
> +   // void getResourceCoords(std::vector<Value *>&, int r, int
s);
>
>      void handleLOAD(Value *dst0[4]);
>      void handleSTORE();
> @@ -2169,6 +2171,7 @@ Converter::handleLIT(Value *dst0[4])
>      }
>   }
>
> +/* Keep this around for now as reference when adding img support
>   static inline bool
>   isResourceSpecial(const int r)
>   {
> @@ -2264,6 +2267,7 @@ partitionLoadStore(uint8_t comp[2], uint8_t size[2],
uint8_t mask)
>      }
>      return n + 1;
>   }
> +*/
>
>   // For raw loads, granularity is 4 byte.
>   // Usage of the texture read mask on OP_SULDP is not allowed.
> @@ -2274,8 +2278,9 @@ Converter::handleLOAD(Value *dst0[4])
>      int c;
>      std::vector<Value *> off, src, ldv, def;
>
> -   if (tgsi.getSrc(0).getFile() == TGSI_FILE_BUFFER ||
> -       tgsi.getSrc(0).getFile() == TGSI_FILE_MEMORY) {
> +   switch (tgsi.getSrc(0).getFile()) {
> +   case TGSI_FILE_BUFFER:
> +   case TGSI_FILE_MEMORY:
>         for (c = 0; c < 4; ++c) {
>            if (!dst0[c])
>               continue;
> @@ -2295,9 +2300,12 @@ Converter::handleLOAD(Value *dst0[4])
>            if (tgsi.getSrc(0).isIndirect(0))
>               ld->setIndirect(0, 1,
fetchSrc(tgsi.getSrc(0).getIndirect(0), 0, 0));
>         }
> -      return;
> +      break;
> +   default:
> +      assert(!"Unsupported srcFile for LOAD");
>      }
>
> +/* Keep this around for now as reference when adding img support
>      getResourceCoords(off, r, 1);
>
>      if (isResourceRaw(code, r)) {
> @@ -2363,6 +2371,7 @@ Converter::handleLOAD(Value *dst0[4])
>      FOR_EACH_DST_ENABLED_CHANNEL(0, c, tgsi)
>         if (dst0[c] != def[c])
>            mkMov(dst0[c], def[tgsi.getSrc(0).getSwizzle(c)]);
> +*/
>   }
>
>   // For formatted stores, the write mask on OP_SUSTP can be used.
> @@ -2374,8 +2383,9 @@ Converter::handleSTORE()
>      int c;
>      std::vector<Value *> off, src, dummy;
>
> -   if (tgsi.getDst(0).getFile() == TGSI_FILE_BUFFER ||
> -       tgsi.getDst(0).getFile() == TGSI_FILE_MEMORY) {
> +   switch (tgsi.getDst(0).getFile()) {
> +   case TGSI_FILE_BUFFER:
> +   case TGSI_FILE_MEMORY:
>         for (c = 0; c < 4; ++c) {
>            if (!(tgsi.getDst(0).getMask() & (1 << c)))
>               continue;
> @@ -2396,9 +2406,12 @@ Converter::handleSTORE()
>            if (tgsi.getDst(0).isIndirect(0))
>               st->setIndirect(0, 1,
fetchSrc(tgsi.getDst(0).getIndirect(0), 0, 0));
>         }
> -      return;
> +      break;
> +   default:
> +      assert(!"Unsupported dstFile for STORE");
>      }
>
> +/* Keep this around for now as reference when adding img support
>      getResourceCoords(off, r, 0);
>      src = off;
>      const int s = src.size();
> @@ -2446,6 +2459,7 @@ Converter::handleSTORE()
>         mkTex(OP_SUSTP, getResourceTarget(code, r),
code->resources[r].slot, 0,
>               dummy, src)->tex.mask = tgsi.getDst(0).getMask();
>      }
> +*/
>   }
>
>   // XXX: These only work on resources with the single-component u32/s32
formats.
> @@ -2460,8 +2474,9 @@ Converter::handleATOM(Value *dst0[4], DataType ty,
uint16_t subOp)
>      std::vector<Value *> defv;
>      LValue *dst = getScratch();
>
> -   if (tgsi.getSrc(0).getFile() == TGSI_FILE_BUFFER ||
> -       tgsi.getSrc(0).getFile() == TGSI_FILE_MEMORY) {
> +   switch (tgsi.getSrc(0).getFile()) {
> +   case TGSI_FILE_BUFFER:
> +   case TGSI_FILE_MEMORY:
>         for (int c = 0; c < 4; ++c) {
>            if (!dst0[c])
>               continue;
> @@ -2489,10 +2504,12 @@ Converter::handleATOM(Value *dst0[4], DataType ty,
uint16_t subOp)
>         for (int c = 0; c < 4; ++c)
>            if (dst0[c])
>               dst0[c] = dst; // not equal to rDst so handleInstruction will
do mkMov
> -      return;
> +      break;
> +   default:
> +      assert(!"Unsupported srcFile for ATOM");
>      }
>
> -
> +/* Keep this around for now as reference when adding img support
>      getResourceCoords(srcv, r, 1);
>
>      if (isResourceSpecial(r)) {
> @@ -2520,6 +2537,7 @@ Converter::handleATOM(Value *dst0[4], DataType ty,
uint16_t subOp)
>      for (int c = 0; c < 4; ++c)
>         if (dst0[c])
>            dst0[c] = dst; // not equal to rDst so handleInstruction will do
mkMov
> +*/
>   }
>
>   void
> diff --git a/src/gallium/include/pipe/p_shader_tokens.h
b/src/gallium/include/pipe/p_shader_tokens.h
> index 65d8ad9..5ef6c30 100644
> --- a/src/gallium/include/pipe/p_shader_tokens.h
> +++ b/src/gallium/include/pipe/p_shader_tokens.h
> @@ -237,15 +237,6 @@ struct tgsi_declaration_array {
>      unsigned Padding : 22;
>   };
>
> -/*
> - * Special resources that don't need to be declared.  They map to the
> - * GLOBAL/LOCAL/PRIVATE/INPUT compute memory spaces.
> - */
> -#define TGSI_RESOURCE_GLOBAL	0x7fff
> -#define TGSI_RESOURCE_LOCAL	0x7ffe
> -#define TGSI_RESOURCE_PRIVATE	0x7ffd
> -#define TGSI_RESOURCE_INPUT	0x7ffc
> -
This should be in a separate patch with "gallium:" as prefix even if 
nouveau is the only driver which somehow uses these constants.

Other than that, the patch looks fine.
And thanks to not remove this resource thing because this could help for 
arb_shader_image_load_store. :-)

I have the same comment as the previous patch, I think the cosmetic 
changes should not be here.
>   #define TGSI_IMM_FLOAT32   0
>   #define TGSI_IMM_UINT32    1
>   #define TGSI_IMM_INT32     2
>

Hi,

On 16-03-16 11:45, Samuel Pitoiset wrote:
>
>
> On 03/16/2016 10:23 AM, Hans de Goede wrote:
>> Commit c3083c7082 ("nv50/ir: add support for BUFFER
accesses") disabled /
>> commented out some of the old resource handling code, but not all of
it.
>>
>> Effectively all of it is dead already, if we ever enter the old code
>> paths in handeLOAD / handleSTORE / handleATOM we will get an exception
>> due to trying to access the now always zero-sized resources vector.
>>
>> Make non buffer / memory file accesses not being supported in these
>> functions more explicit and comment out a whole bunch of dead code.
>>
>> Also remove the magic file-indexe defines from the old resource code
>> from include/pipe/p_shader_tokens.h as those are no longer used now
>> (which is a good thing).
>>
>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>> ---
>>   .../drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp  | 42
+++++++++++++++-------
>>   src/gallium/include/pipe/p_shader_tokens.h         |  9 -----
>>   2 files changed, 30 insertions(+), 21 deletions(-)
>>
>> diff --git a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>> index c167c4a..115d0bb 100644
>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>> @@ -856,12 +856,14 @@ public:
>>      };
>>      std::vector<TextureView> textureViews;
>>
>> +   /*
>>      struct Resource {
>>         uint8_t target; // TGSI_TEXTURE_*
>>         bool raw;
>>         uint8_t slot; // $surface index
>>      };
>>      std::vector<Resource> resources;
>> +   */
>>
>>      struct MemoryFile {
>>         uint8_t mem_type; // TGSI_MEMORY_TYPE_*
>> @@ -1423,8 +1425,8 @@ private:
>>      void handleLIT(Value *dst0[4]);
>>      void handleUserClipPlanes();
>>
>> -   Symbol *getResourceBase(int r);
>> -   void getResourceCoords(std::vector<Value *>&, int r, int
s);
>> +   // Symbol *getResourceBase(int r);
>> +   // void getResourceCoords(std::vector<Value *>&, int r,
int s);
>>
>>      void handleLOAD(Value *dst0[4]);
>>      void handleSTORE();
>> @@ -2169,6 +2171,7 @@ Converter::handleLIT(Value *dst0[4])
>>      }
>>   }
>>
>> +/* Keep this around for now as reference when adding img support
>>   static inline bool
>>   isResourceSpecial(const int r)
>>   {
>> @@ -2264,6 +2267,7 @@ partitionLoadStore(uint8_t comp[2], uint8_t
size[2], uint8_t mask)
>>      }
>>      return n + 1;
>>   }
>> +*/
>>
>>   // For raw loads, granularity is 4 byte.
>>   // Usage of the texture read mask on OP_SULDP is not allowed.
>> @@ -2274,8 +2278,9 @@ Converter::handleLOAD(Value *dst0[4])
>>      int c;
>>      std::vector<Value *> off, src, ldv, def;
>>
>> -   if (tgsi.getSrc(0).getFile() == TGSI_FILE_BUFFER ||
>> -       tgsi.getSrc(0).getFile() == TGSI_FILE_MEMORY) {
>> +   switch (tgsi.getSrc(0).getFile()) {
>> +   case TGSI_FILE_BUFFER:
>> +   case TGSI_FILE_MEMORY:
>>         for (c = 0; c < 4; ++c) {
>>            if (!dst0[c])
>>               continue;
>> @@ -2295,9 +2300,12 @@ Converter::handleLOAD(Value *dst0[4])
>>            if (tgsi.getSrc(0).isIndirect(0))
>>               ld->setIndirect(0, 1,
fetchSrc(tgsi.getSrc(0).getIndirect(0), 0, 0));
>>         }
>> -      return;
>> +      break;
>> +   default:
>> +      assert(!"Unsupported srcFile for LOAD");
>>      }
>>
>> +/* Keep this around for now as reference when adding img support
>>      getResourceCoords(off, r, 1);
>>
>>      if (isResourceRaw(code, r)) {
>> @@ -2363,6 +2371,7 @@ Converter::handleLOAD(Value *dst0[4])
>>      FOR_EACH_DST_ENABLED_CHANNEL(0, c, tgsi)
>>         if (dst0[c] != def[c])
>>            mkMov(dst0[c], def[tgsi.getSrc(0).getSwizzle(c)]);
>> +*/
>>   }
>>
>>   // For formatted stores, the write mask on OP_SUSTP can be used.
>> @@ -2374,8 +2383,9 @@ Converter::handleSTORE()
>>      int c;
>>      std::vector<Value *> off, src, dummy;
>>
>> -   if (tgsi.getDst(0).getFile() == TGSI_FILE_BUFFER ||
>> -       tgsi.getDst(0).getFile() == TGSI_FILE_MEMORY) {
>> +   switch (tgsi.getDst(0).getFile()) {
>> +   case TGSI_FILE_BUFFER:
>> +   case TGSI_FILE_MEMORY:
>>         for (c = 0; c < 4; ++c) {
>>            if (!(tgsi.getDst(0).getMask() & (1 << c)))
>>               continue;
>> @@ -2396,9 +2406,12 @@ Converter::handleSTORE()
>>            if (tgsi.getDst(0).isIndirect(0))
>>               st->setIndirect(0, 1,
fetchSrc(tgsi.getDst(0).getIndirect(0), 0, 0));
>>         }
>> -      return;
>> +      break;
>> +   default:
>> +      assert(!"Unsupported dstFile for STORE");
>>      }
>>
>> +/* Keep this around for now as reference when adding img support
>>      getResourceCoords(off, r, 0);
>>      src = off;
>>      const int s = src.size();
>> @@ -2446,6 +2459,7 @@ Converter::handleSTORE()
>>         mkTex(OP_SUSTP, getResourceTarget(code, r),
code->resources[r].slot, 0,
>>               dummy, src)->tex.mask = tgsi.getDst(0).getMask();
>>      }
>> +*/
>>   }
>>
>>   // XXX: These only work on resources with the single-component
u32/s32 formats.
>> @@ -2460,8 +2474,9 @@ Converter::handleATOM(Value *dst0[4], DataType
ty, uint16_t subOp)
>>      std::vector<Value *> defv;
>>      LValue *dst = getScratch();
>>
>> -   if (tgsi.getSrc(0).getFile() == TGSI_FILE_BUFFER ||
>> -       tgsi.getSrc(0).getFile() == TGSI_FILE_MEMORY) {
>> +   switch (tgsi.getSrc(0).getFile()) {
>> +   case TGSI_FILE_BUFFER:
>> +   case TGSI_FILE_MEMORY:
>>         for (int c = 0; c < 4; ++c) {
>>            if (!dst0[c])
>>               continue;
>> @@ -2489,10 +2504,12 @@ Converter::handleATOM(Value *dst0[4], DataType
ty, uint16_t subOp)
>>         for (int c = 0; c < 4; ++c)
>>            if (dst0[c])
>>               dst0[c] = dst; // not equal to rDst so handleInstruction
will do mkMov
>> -      return;
>> +      break;
>> +   default:
>> +      assert(!"Unsupported srcFile for ATOM");
>>      }
>>
>> -
>> +/* Keep this around for now as reference when adding img support
>>      getResourceCoords(srcv, r, 1);
>>
>>      if (isResourceSpecial(r)) {
>> @@ -2520,6 +2537,7 @@ Converter::handleATOM(Value *dst0[4], DataType
ty, uint16_t subOp)
>>      for (int c = 0; c < 4; ++c)
>>         if (dst0[c])
>>            dst0[c] = dst; // not equal to rDst so handleInstruction
will do mkMov
>> +*/
>>   }
>>
>>   void
>> diff --git a/src/gallium/include/pipe/p_shader_tokens.h
b/src/gallium/include/pipe/p_shader_tokens.h
>> index 65d8ad9..5ef6c30 100644
>> --- a/src/gallium/include/pipe/p_shader_tokens.h
>> +++ b/src/gallium/include/pipe/p_shader_tokens.h
>> @@ -237,15 +237,6 @@ struct tgsi_declaration_array {
>>      unsigned Padding : 22;
>>   };
>>
>> -/*
>> - * Special resources that don't need to be declared.  They map to
the
>> - * GLOBAL/LOCAL/PRIVATE/INPUT compute memory spaces.
>> - */
>> -#define TGSI_RESOURCE_GLOBAL    0x7fff
>> -#define TGSI_RESOURCE_LOCAL    0x7ffe
>> -#define TGSI_RESOURCE_PRIVATE    0x7ffd
>> -#define TGSI_RESOURCE_INPUT    0x7ffc
>> -
>
> This should be in a separate patch with "gallium:" as prefix even
if nouveau is the only driver which somehow uses these constants.
Ok, will do.
> Other than that, the patch looks fine.
> And thanks to not remove this resource thing because this could help for
arb_shader_image_load_store. :-)
>
> I have the same comment as the previous patch, I think the cosmetic changes
should not be here.
You mean the changing from if (... || ...) to switch case ? That is not
cosmetic, note the
new default: code path with the assert(). This replaces the implicit assert we
had before by
in the form of the old resource code throwing an exceptions because of the code
indexing
a 0 size vector.

This is the part of the commit described by this bit of the commit msg:

"Make non buffer / memory file accesses not being supported in these
functions more explicit and comment out a whole bunch of dead code."

I could split this into a separate commit if you want me to.

Regards,

Hans

On 03/16/2016 11:49 AM, Hans de Goede wrote:
> Hi,
>
> On 16-03-16 11:45, Samuel Pitoiset wrote:
>>
>>
>> On 03/16/2016 10:23 AM, Hans de Goede wrote:
>>> Commit c3083c7082 ("nv50/ir: add support for BUFFER
accesses")
>>> disabled /
>>> commented out some of the old resource handling code, but not all
of it.
>>>
>>> Effectively all of it is dead already, if we ever enter the old
code
>>> paths in handeLOAD / handleSTORE / handleATOM we will get an
exception
>>> due to trying to access the now always zero-sized resources vector.
>>>
>>> Make non buffer / memory file accesses not being supported in these
>>> functions more explicit and comment out a whole bunch of dead code.
>>>
>>> Also remove the magic file-indexe defines from the old resource
code
>>> from include/pipe/p_shader_tokens.h as those are no longer used now
>>> (which is a good thing).
>>>
>>> Signed-off-by: Hans de Goede <hdegoede at redhat.com>
>>> ---
>>>   .../drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp  | 42
>>> +++++++++++++++-------
>>>   src/gallium/include/pipe/p_shader_tokens.h         |  9 -----
>>>   2 files changed, 30 insertions(+), 21 deletions(-)
>>>
>>> diff --git
>>> a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>> b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>> index c167c4a..115d0bb 100644
>>> --- a/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>> +++ b/src/gallium/drivers/nouveau/codegen/nv50_ir_from_tgsi.cpp
>>> @@ -856,12 +856,14 @@ public:
>>>      };
>>>      std::vector<TextureView> textureViews;
>>>
>>> +   /*
>>>      struct Resource {
>>>         uint8_t target; // TGSI_TEXTURE_*
>>>         bool raw;
>>>         uint8_t slot; // $surface index
>>>      };
>>>      std::vector<Resource> resources;
>>> +   */
>>>
>>>      struct MemoryFile {
>>>         uint8_t mem_type; // TGSI_MEMORY_TYPE_*
>>> @@ -1423,8 +1425,8 @@ private:
>>>      void handleLIT(Value *dst0[4]);
>>>      void handleUserClipPlanes();
>>>
>>> -   Symbol *getResourceBase(int r);
>>> -   void getResourceCoords(std::vector<Value *>&, int r,
int s);
>>> +   // Symbol *getResourceBase(int r);
>>> +   // void getResourceCoords(std::vector<Value *>&, int
r, int s);
>>>
>>>      void handleLOAD(Value *dst0[4]);
>>>      void handleSTORE();
>>> @@ -2169,6 +2171,7 @@ Converter::handleLIT(Value *dst0[4])
>>>      }
>>>   }
>>>
>>> +/* Keep this around for now as reference when adding img support
>>>   static inline bool
>>>   isResourceSpecial(const int r)
>>>   {
>>> @@ -2264,6 +2267,7 @@ partitionLoadStore(uint8_t comp[2], uint8_t
>>> size[2], uint8_t mask)
>>>      }
>>>      return n + 1;
>>>   }
>>> +*/
>>>
>>>   // For raw loads, granularity is 4 byte.
>>>   // Usage of the texture read mask on OP_SULDP is not allowed.
>>> @@ -2274,8 +2278,9 @@ Converter::handleLOAD(Value *dst0[4])
>>>      int c;
>>>      std::vector<Value *> off, src, ldv, def;
>>>
>>> -   if (tgsi.getSrc(0).getFile() == TGSI_FILE_BUFFER ||
>>> -       tgsi.getSrc(0).getFile() == TGSI_FILE_MEMORY) {
>>> +   switch (tgsi.getSrc(0).getFile()) {
>>> +   case TGSI_FILE_BUFFER:
>>> +   case TGSI_FILE_MEMORY:
>>>         for (c = 0; c < 4; ++c) {
>>>            if (!dst0[c])
>>>               continue;
>>> @@ -2295,9 +2300,12 @@ Converter::handleLOAD(Value *dst0[4])
>>>            if (tgsi.getSrc(0).isIndirect(0))
>>>               ld->setIndirect(0, 1,
>>> fetchSrc(tgsi.getSrc(0).getIndirect(0), 0, 0));
>>>         }
>>> -      return;
>>> +      break;
>>> +   default:
>>> +      assert(!"Unsupported srcFile for LOAD");
>>>      }
>>>
>>> +/* Keep this around for now as reference when adding img support
>>>      getResourceCoords(off, r, 1);
>>>
>>>      if (isResourceRaw(code, r)) {
>>> @@ -2363,6 +2371,7 @@ Converter::handleLOAD(Value *dst0[4])
>>>      FOR_EACH_DST_ENABLED_CHANNEL(0, c, tgsi)
>>>         if (dst0[c] != def[c])
>>>            mkMov(dst0[c], def[tgsi.getSrc(0).getSwizzle(c)]);
>>> +*/
>>>   }
>>>
>>>   // For formatted stores, the write mask on OP_SUSTP can be used.
>>> @@ -2374,8 +2383,9 @@ Converter::handleSTORE()
>>>      int c;
>>>      std::vector<Value *> off, src, dummy;
>>>
>>> -   if (tgsi.getDst(0).getFile() == TGSI_FILE_BUFFER ||
>>> -       tgsi.getDst(0).getFile() == TGSI_FILE_MEMORY) {
>>> +   switch (tgsi.getDst(0).getFile()) {
>>> +   case TGSI_FILE_BUFFER:
>>> +   case TGSI_FILE_MEMORY:
>>>         for (c = 0; c < 4; ++c) {
>>>            if (!(tgsi.getDst(0).getMask() & (1 << c)))
>>>               continue;
>>> @@ -2396,9 +2406,12 @@ Converter::handleSTORE()
>>>            if (tgsi.getDst(0).isIndirect(0))
>>>               st->setIndirect(0, 1,
>>> fetchSrc(tgsi.getDst(0).getIndirect(0), 0, 0));
>>>         }
>>> -      return;
>>> +      break;
>>> +   default:
>>> +      assert(!"Unsupported dstFile for STORE");
>>>      }
>>>
>>> +/* Keep this around for now as reference when adding img support
>>>      getResourceCoords(off, r, 0);
>>>      src = off;
>>>      const int s = src.size();
>>> @@ -2446,6 +2459,7 @@ Converter::handleSTORE()
>>>         mkTex(OP_SUSTP, getResourceTarget(code, r),
>>> code->resources[r].slot, 0,
>>>               dummy, src)->tex.mask = tgsi.getDst(0).getMask();
>>>      }
>>> +*/
>>>   }
>>>
>>>   // XXX: These only work on resources with the single-component
>>> u32/s32 formats.
>>> @@ -2460,8 +2474,9 @@ Converter::handleATOM(Value *dst0[4],
DataType
>>> ty, uint16_t subOp)
>>>      std::vector<Value *> defv;
>>>      LValue *dst = getScratch();
>>>
>>> -   if (tgsi.getSrc(0).getFile() == TGSI_FILE_BUFFER ||
>>> -       tgsi.getSrc(0).getFile() == TGSI_FILE_MEMORY) {
>>> +   switch (tgsi.getSrc(0).getFile()) {
>>> +   case TGSI_FILE_BUFFER:
>>> +   case TGSI_FILE_MEMORY:
>>>         for (int c = 0; c < 4; ++c) {
>>>            if (!dst0[c])
>>>               continue;
>>> @@ -2489,10 +2504,12 @@ Converter::handleATOM(Value *dst0[4],
>>> DataType ty, uint16_t subOp)
>>>         for (int c = 0; c < 4; ++c)
>>>            if (dst0[c])
>>>               dst0[c] = dst; // not equal to rDst so
>>> handleInstruction will do mkMov
>>> -      return;
>>> +      break;
>>> +   default:
>>> +      assert(!"Unsupported srcFile for ATOM");
>>>      }
>>>
>>> -
>>> +/* Keep this around for now as reference when adding img support
>>>      getResourceCoords(srcv, r, 1);
>>>
>>>      if (isResourceSpecial(r)) {
>>> @@ -2520,6 +2537,7 @@ Converter::handleATOM(Value *dst0[4],
DataType
>>> ty, uint16_t subOp)
>>>      for (int c = 0; c < 4; ++c)
>>>         if (dst0[c])
>>>            dst0[c] = dst; // not equal to rDst so handleInstruction
>>> will do mkMov
>>> +*/
>>>   }
>>>
>>>   void
>>> diff --git a/src/gallium/include/pipe/p_shader_tokens.h
>>> b/src/gallium/include/pipe/p_shader_tokens.h
>>> index 65d8ad9..5ef6c30 100644
>>> --- a/src/gallium/include/pipe/p_shader_tokens.h
>>> +++ b/src/gallium/include/pipe/p_shader_tokens.h
>>> @@ -237,15 +237,6 @@ struct tgsi_declaration_array {
>>>      unsigned Padding : 22;
>>>   };
>>>
>>> -/*
>>> - * Special resources that don't need to be declared.  They map
to the
>>> - * GLOBAL/LOCAL/PRIVATE/INPUT compute memory spaces.
>>> - */
>>> -#define TGSI_RESOURCE_GLOBAL    0x7fff
>>> -#define TGSI_RESOURCE_LOCAL    0x7ffe
>>> -#define TGSI_RESOURCE_PRIVATE    0x7ffd
>>> -#define TGSI_RESOURCE_INPUT    0x7ffc
>>> -
>>
>> This should be in a separate patch with "gallium:" as prefix
even if
>> nouveau is the only driver which somehow uses these constants.
>
> Ok, will do.
>
>> Other than that, the patch looks fine.
>> And thanks to not remove this resource thing because this could help
>> for arb_shader_image_load_store. :-)
>>
>> I have the same comment as the previous patch, I think the cosmetic
>> changes should not be here.
>
> You mean the changing from if (... || ...) to switch case ? That is not
> cosmetic, note the
> new default: code path with the assert(). This replaces the implicit
> assert we had before by
> in the form of the old resource code throwing an exceptions because of
> the code indexing
> a 0 size vector.
>
> This is the part of the commit described by this bit of the commit msg:
>
> "Make non buffer / memory file accesses not being supported in these
> functions more explicit and comment out a whole bunch of dead code."
>
> I could split this into a separate commit if you want me to.
Yeah, seems like better.
Thanks.
>
> Regards,
>
> Hans