Bug 8283

Created attachment 6989 [details] [review]
Add swizzle support for TEX FP op.

Firstly, the approach looks like a good one, but a couple of comments:  

    - Please use universal diffs (diff -u) as they're easier for others to read...

    - It looks like you don't seem to cope with the situation where
get_free_rreg() fails, ie. the return value isn't checked.

    - There should already be a 'NumInstructions' field hanging around, so you
shouldn't need to scan the list twice.

    - It would be good to move the calculation of temporaries used into core
mesa somewhere, perhaps taking the regsNeeded array as an argument.  But I don't
think this is a big deal at this point.

Otherwise, it looks good - nice work.

Actually I think the calculation of neededRegs is incorrect, consider:

MOV B, 99
..
MOV C, B
TEX X, Y.zyx
MOV B, B
MOV A, B

Working backwards, I think that after MOV B, B the fact that B is still live
will be lost, so regsNeeded at the TEX instruction will be incorrect and B could
be used as a temporary.

Created attachment 6993 [details] [review]
Add swizzle support for TEX FP op. (try 2)

Incorporated feedback from comments. The case in comment #3 should now be
handled properly, I think.

Created attachment 7095 [details] [review]
TEXLD swizzle nr 3

Does this version work for you?  I've slightly cleaned things up:
  - move calculate into its own function
  - use the existing mesa call for the number of argments of an instruction
  - just pass the bitmask to emit_texld so that code doesn't learn too much
about mesa's fragment program representation.

Keith

The bug priority was upgraded (P2->high) with the bugzilla configuration change.
I'm Changing the priority back to the normal one.
Sorry for the spam.

any response, Frank?

*** Bug 10786 has been marked as a duplicate of this bug. ***

The latest (In reply to comment #5)
> Created an attachment (id=7095) [details]
> TEXLD swizzle nr 3
> 
This patch works fine for me and I check it into git (3369cd9a6f943365242d7832e69788d4aede9a8f)

*** Bug 14122 has been marked as a duplicate of this bug. ***

Haihao, the fix is not at mesa_7_0_branch. Please pick it up. Thanks!

(In reply to comment #12)
> Haihao, the fix is not at mesa_7_0_branch. Please pick it up. Thanks!
> 
Done

Still doesn't work for me (Using the 7.0.3 mesa driver version bundled with OpenSuSE 11.0)

Also I can't seem to be able to use your "patches" (although I'm about 98% sure I'm doing something wrong - file saved as "patch.cgi", chmoded to 764) Here is the error I'm getting:

./patch.cgi: line 1: ?: command not found
./patch.cgi: line 2: ?: command not found
./patch.cgi: line 3: ?: command not found
./patch.cgi: line 4: ?: command not found
./patch.cgi: line 5: Index:: command not found
./patch.cgi: line 6: ===================================================================: command not found
./patch.cgi: line 7: RCS: command not found
./patch.cgi: line 8: retrieving: command not found
diff: invalid option -- '.'
diff: Try `diff --help' for more information.
./patch.cgi: line 10: ---: command not found
./patch.cgi: line 11: +++: command not found
./patch.cgi: line 12: @@: command not found
./patch.cgi: line 16: +#include: command not found
./patch.cgi: line 20: @@: command not found
./patch.cgi: line 24: syntax error near unexpected token `('
./patch.cgi: line 24: `+#define I915_MAX_INSN          (I915_MAX_TEX_INSN+I915_MAX_ALU_INSN)'

Could someone tell me what I'm doing wrong / suggest another fix / do I have have to file a bug report for the patch (don't think so, though: seems to be my fault...)

This patch has been pushed into mesa_7_0_branch. 

Mass version move, cvs -> git