[Mesa-dev] [PATCH mesa 6/6] nouveau: codegen: Disable more old resource handling code

Samuel Pitoiset samuel.pitoiset at gmail.com
Wed Mar 16 12:20:17 UTC 2016



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


More information about the mesa-dev mailing list