2012/1/24 Vadim Girlin <vadimgir...@gmail.com>: > On Mon, 2012-01-23 at 14:20 +0100, Christian König wrote: >> On 22.01.2012 17:24, Dave Airlie wrote: >> > 2012/1/22 Christian König<deathsim...@vodafone.de>: >> >> On 22.01.2012 16:46, Dave Airlie wrote: >> >>> 2012/1/22 Christian König<deathsim...@vodafone.de>: >> >>>> Sorry, but that looks really ugly and pretty much unmaintainable, cause >> >>>> you >> >>>> constantly need to lookup the meaning of the values. >> >>>> >> >>>> Also I haven't looked into the docs (but going to do so tomorrow), but >> >>>> I'm >> >>>> pretty sure that those ranges aren't 100% correct. >> >>> It was the docs that the ranges came from, and we keep forgetting to >> >>> add things to these lookup functions. >> >> >> >> Really? Where? I asked around when I coded this in the first place if it >> >> could be simplified by using ranges, but never got an clear answer on this >> >> topic. >> >> >> >> When I now look into the AMD docs they mostly seems to use tables for >> >> opcode >> >> attributes, so I assumed that they are spread around in the opcode range. >> >> >> >> If this isn't the case then this indeed seems to be an good idea. >> > Its in the Evergreen ISA docs (it might only be evergreen they managed >> > this). >> > >> > ALU_WORD1_OP2 >> > ALU_INST >> > [17:7] >> > enum(11) >> > Instruction. The top three bits of this field must be zero. Gaps in >> > opcode values >> > are not marked in the list below. See Chapter 7 for descriptions of each >> > instruction. Opcodes 0..95 can be used in either the Vector or Trans unit. >> > Opcodes 128..159 are Trans only. Opcodes 160..255 are vector only. >> Those ranges are even better than the table based documentation! The >> tables contained an quite ugly bug which lead to this comment in the >> original code: >> >> /* Note that FLT_TO_INT_* instructions are vector-only instructions >> * on Evergreen, despite what the documentation says. FLT_TO_INT >> * can do both vector and scalar. */ >> >> Very interesting, but I couldn't such ranges for R6xx and R7xx. Alex do >> you remember where you got that original documentation from? >> >> Anyway I would still suggest to not use magic numbers, let's define the >> beginning and end of ranges in r600_opcode.h instead, and then use those >> defines inside the code. > > I'm not sure which names we could choose for that, and if this will make > things more clear and readable. I think at least for me it's much easier > to compare some opcode against the numeric ranges, than to look for > names/values in the additional lists/headers/etc.
It's for people who don't know the hardware to the letter and who want to know what the magic numbers stand for. The code already functions as a secondary documentation to the hardware. Some people, me including, first take a look at how r600g does things and then look up the official docs for more precise info. Not everything is properly documented and the documentation we have is scattered across a lot of PDFs, so having self-documenting code is really important. Also, "RANGE(a,b)" isn't very informative and it's very ugly to use variables in a macro which are not parameters to the macro (in this case, it's "alu"). I know such macros have started to creep into r600g, but that doesn't necessarily set a precedent. Please turn it into a static function with a proper name e.g. bool is_opcode_in_range(opcode, min, max). For min and max, you can use the definitions of opcodes directly. People can then look up the opcode definitions and see which opcodes are in between. Marek _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
