On 12.03.2013 12:10, Christoph Bumiller wrote: > On 12.03.2013 10:31, Christian König wrote: >> Am 12.03.2013 02:48, schrieb Marek Olšák: >>> On Mon, Mar 11, 2013 at 1:44 PM, Christian König >>> <deathsim...@vodafone.de> wrote: >>>> Hi everybody, >>>> >>>> this problem has been open for quite some time now, with a bunch of >>>> different >>>> opinions and sometimes even patches floating on the list. >>>> >>>> The solutions proposed or implemented so far all more or less >>>> incomplete, so >>>> this approach was designed in mind with both completeness and >>>> compatibility >>>> with existing code. >>>> >>>> Over all it's just an implementation of what Tom Stellard named >>>> solution #4 in >>>> this eMail thread: >>>> http://lists.freedesktop.org/archives/mesa-dev/2013-January/033264.html >>> Hi Christian, >>> >>> this is definitely not the solution #4. According to the TGSI dump >>> Christoph posted, it looks more like #3. >> Well, for me the main difference between proposal #3 and #4 is that #3 >> tries to identify the declaration to use with the supplied "offset", >> while #4 uses a completely distinct identifier for that. >> >>> The solution #4 completely changes the temporary file such that it >>> becomes two-dimensional with the first index being a literal and the >>> second index being either a literal or ADDR[literal], and it would >>> always be like that regardless of whether drivers support that or not. >>> One-dimensional indexing of TEMP is not allowed. For backward >>> compatibility, the drivers that do not support it would only get a >>> single array declaration TEMP[0][0..n] and TEMP[0][...] would be >>> everywhere in the code. >> Ok, then I misunderstood you a bit, but I don't think the difference >> is so much. >> >> What I'm proposing is that we have an optional "ArrayID" attached to >> each declaration and refer to this "ArrayID" in the indirect >> addressing operand. To sum it up declarations should look something >> like this: >> >> DCL TEMP[0..3] // normal registers >> DCL TEMP[1][4..11] // indirectly accessed array >> DCL TEMP[2][12..15] // another indirectly accessed array >> DCL TEMP[16..17] LOCAL // local registers >> >> While an indirect operand might look like this: >> >> MOV TEMP[16], TEMP[1][ADDR[0].x-13] >> >> On the pro side for this approach is that it is compatible with all >> the existing state trackers and driver, and we don't need to generate >> different code depending on weather or not the driver supports this. >> >>> I don't know much about TGSI internals, so I can't review this. I'd >>> just like to say that TGSI dumps should make sense (2D indexing should >>> be only allowed with 2D declarations) and tgsi_text_translate should >>> be able to do the reverse - convert the dumps back to TGSI tokens. >> Completely agree with that, and beside writing documentation testing >> this is still one of the todos with this patchset. >> >> I have to admit that your approach looks a bit cleaner from the high >> above view. The problem with it is that it requires this additional 2D >> index on every operand, and we just don't have enough bits left for >> this. Even with my approach I need to make room for this ArrayID in >> the indirect addressing operand token, and this additional token is >> only there if the operand uses indirect adressing. >> >> Do you think we can live with my approach or is there any major >> downside I currently don't see? >>
One more thing. While you're at it (i.e. are familiar with the code), could you set the UsageMask in the TGSI declaration so we can pack scalar or vec2 arrays ? Also, you could then declare gl_ClipDistance outputs as DCL OUT[0..7].x, CLIPDIST so we can actually index clip distances properly ? With DCL OUT[0..1].xyzw, CLIPDIST we can't really index the individual components which leads to if ((index & 3) == 0) MOV OUT[index / 4].x = value else if ((index & 3) == 1) MOV OUT[index / 4].y = value which is unnecessary on some hardware. > I can live with it. I think ... (I hope I don't regret this later; seems > like this doesn't contain less information, then it's ok.) > If the placement of the hint index offends someone, just write it as > "MOV TEMP[16], TEMP(1)[ADDR[0].x-13]" or ... > TEMP[ADDR[0].x-13 : 1] or > TEMP[ADDR[0].x-13 supposedToBeIn [4,11]] or > something ... nicer. > > Actually ... > if TEMP[0] is placed at mem[0] > and TEMP[4..1] is placed at, say, mem[0x1000 in bytes] > > do I have to > load $register mem[$addr - 0xd0] (no this can't work) or > load $regsiter mem[$addr - 0xd0 + 0x1000] (if you didn't adjust the > offset) or > load $register mem[$addr - 0xd0 + 0x1000 - 0x40] (if you already added > the base TEMP to the immediate offset) > > This needs to be documented as well. > >> Thanks for the clarification, >> Christian. >> _______________________________________________ >> mesa-dev mailing list >> mesa-dev@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/mesa-dev >> > _______________________________________________ > mesa-dev mailing list > mesa-dev@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/mesa-dev > _______________________________________________ mesa-dev mailing list mesa-dev@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/mesa-dev
