[Mesa-dev] [PATCH 2/3] r600g: replace trans/vector-only instruction lists with ranges

Marek Olšák maraeo at gmail.com
Mon Jan 23 15:44:17 PST 2012

2012/1/24 Vadim Girlin <vadimgirlin at 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<deathsimple at vodafone.de>:
>> >> On 22.01.2012 16:46, Dave Airlie wrote:
>> >>> 2012/1/22 Christian König<deathsimple at 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
>> > [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.


More information about the mesa-dev mailing list