[Mesa-dev] [PATCH mesa 6/6] nouveau: codegen: Disable more old resource handling code
Hans de Goede
hdegoede at redhat.com
Wed Mar 16 10:49:34 UTC 2016
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
More information about the mesa-dev
mailing list