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

[Marek Olšák]

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
>> > 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