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

[Vadim Girlin]

On Tue, 2012-01-24 at 00:44 +0100, Marek Olšák wrote:
> 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.
> 

OK, let's imagine I want to check e.g. instruction 0x60 LOG_IEEE for
allowed slots. What could I think looking at the following:

	is_opcode_in_range(alu->inst,
		V_SQ_ALU_WORD1_OP2_SQ_OP2_INST_MOVA_GPR_INT,
		V_SQ_ALU_WORD1_OP2_SQ_OP2_INST_COS)

Well, if it's informative, I'm OK with it or any other solution that is
preferable for the most of the developers. I'll send a patch.

Vadim




> Marek