[Nouveau] [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 Nouveau
mailing list