Hello Nicolai,
Am Sonntag, den 18.06.2017, 12:05 +0200 schrieb Nicolai Hähnle:
> if HAVE_XLIB_GLX
> SUBDIRS += drivers/x11
> @@ -101,7 +101,7 @@ AM_CFLAGS = \
> $(VISIBILITY_CFLAGS) \
> $(MSVC2013_COMPAT_CFLAGS)
> AM_CXXFLAGS = \
> - $(LLVM_CFLAGS) \
> + $(LLVM_CXXFLAGS) \
I kind of suspect that this might be a no-no. On the one hand it
makes sense, because it makes the use of CXXFLAGS consistent, but
it's a pretty significant build system change.
I understand that, but c++11 is just such an improvement.
At least separate it out into its own patch.
Okay,
> $(VISIBILITY_CXXFLAGS) \
> $(MSVC2013_COMPAT_CXXFLAGS)
>
> diff --git a/src/mesa/Makefile.sources b/src/mesa/Makefile.sources
> index 21f9167bda..a68e9d2afe 100644
> --- a/src/mesa/Makefile.sources
> +++ b/src/mesa/Makefile.sources
> @@ -509,6 +509,8 @@ STATETRACKER_FILES = \
> state_tracker/st_glsl_to_tgsi.h \
> state_tracker/st_glsl_to_tgsi_private.cpp \
> state_tracker/st_glsl_to_tgsi_private.h \
> + state_tracker/st_glsl_to_tgsi_temprename.cpp \
> + state_tracker/st_glsl_to_tgsi_temprename.h \
Inconsistent use of whitespace.
Then the whole patch needs to be re-arranged, i.e. the
st_glsl_to_tgsi_private stuff,
This was a mistake when I was re-basing the patch.
and I agree with Emil that the tests
should be separated out into their own patch.
I'm currently working on this.
BTW, you can and should use
git rebase -x "cd $builddir && make" $basecommit
to verify that you don't break the build in an intermediate patch.
Nice trick, thanks.
> +typedef int scope_idx;
Please remove this, it's an unnecessary and distracting abstraction
that doesn't gain you anything.
Actually, with the refactoring it did the last two days it helped a
lot.
I consider the scopes back-reference here and in temp_access to be
bad style.
I already got rid of them.
Think less object- and more data-oriented, then you won't need them.
Most of those helper functions should be members of
tgsi_temp_lifetime -- and the getters/setters can be removed
entirely. YAGNI.
I think with the new design that uses pointers into a pre-allocated
array the methods must stay where they are.
To first approximation, you should never use mutable. Those methods
should not be const to begin with.
This is something else I actually didn't like and corrected already.
> +scope_idx
> +tgsi_temp_lifetime::make_scope(e_scope_type type, int id, int lvl,
> + int s_begin)const
> +{
> + int idx = scopes.size();
> + scopes.push_back(prog_scope(type, idx, id, lvl, s_begin,
> scopes));
> + return idx;
> +}
AFAICS this overload is only used once. Please remove it.
Okay.
Use C-style comments.
Okay,
Also, this should probably have an assert, or can this really happen?
I don't think it can happen, an assert it is.
+ case TGSI_OPCODE_IF:
> + case TGSI_OPCODE_UIF:{
> + if (inst->src[0].file == PROGRAM_TEMPORARY) {
> + acc[inst->src[0].index].append(line, acc_read,
> current);
> + }
> + scope_idx scope = make_scope(current, sct_if, if_id,
> nesting_lvl, line);
It's probably a good idea to have scopes start at line + 1.
Otherwise,
it may be tempting to think that the read access of the IF condition
happens inside the IF scope, at least based on the line numbers.
I don't think that's a problem. At least I can't think of a test case
that would trigger a problem with that.
> + case TGSI_OPCODE_DEFAULT: {
> + auto scope_type = (inst->op == TGSI_OPCODE_CASE) ?
> + sct_switch_case :
> sct_switch_default;
> + if ( scopes[current].type() == sct_switch ) {
No spaces inside parenthesis (also elsewhere).
Okay.
> + scope_stack.push(current);
> + current = make_scope(current, scope_type,
> scopes[current].id(),
> + nesting_lvl, line);
> + }else{
Spaces around else (also elsewhere).
Okay.
No comment between if- and else-block.
Okay.
> + scopes[current].set_continue(current, line);
You do need to distinguish between break and continue (at least for
accuracy), because break allows you to skip over a piece of code
indefinitely while continue doesn't. I.e.:
I risk to differ, in the worst case a CONT can act like it would be a
BRK, and because I have to handle the worst case it doesn't make sense
to distinguish between the two.
BGNLOOP
...
BRK/CONT
...
MOV TEMP[i], ...
...
ENDLOOP
If it's a CONT, then i is written unconditionally inside the loop.
While one would normally expect this, it is not guarantied. Think of a
shader like this:
...
varying int a;
varying float b;
int f(int a, float b) {...}
for (int i = 0; i< n; ++i ) {
if (a && f(i, b))
continue;
...
x =
}
It may not be the best style. but it is possible.
+
> + for (unsigned j = 0; j < inst->tex_offset_num_offset;
> j++) {
> + if (inst->tex_offsets[j].file == PROGRAM_TEMPORARY) {
> + acc[inst->tex_offsets[j].index].append(line,
> acc_read, current);
> + }
> + }
You need to handle the reads before the writes. After all, you might
have a
ADD TEMP[i], TEMP[i], ...
which may have to register as an undefined read.
I will add a test case, and correct the code accordingly.
> +
> + } // end default
> + } // end switch (op)
No end-of-scope comments.
okay,
>
> +scope_idx
> +prog_scope::get_parent_loop() const
Based on what it does, this should be renamed to something like
get_innermost_loop.
Okay.
> +void temp_access::append(int line, e_acc_type acc, scope_idx idx)
> +{
> + last_write = line;
Looks like last_write should be called last_access.
Initially I thought the same, but a last read is also (most of the
time) the last access, and it is handled differently then a write past
the last read, so i think that last_write catches it better.
> + if (acc == acc_read) {
> + last_read = line;
> + last_read_scope = idx;
> + if (undefined_read > line) {
> + undefined_read = line;
> + undefined_read_scope = idx;
> + }
The way you're using it this is effectively just first_read, not
undefined_read.
Indeed.
> + } else {
> + if (first_write == -1) {
> + first_write = line;
> + first_write_scope = idx;
> +
> + // we write in an if-branch
> + auto fw_ifthen_scope = scopes[idx].in_ifelse();
> + if ((fw_ifthen_scope >= 0) &&
> scopes[fw_ifthen_scope].in_loop()) {
> + // value not always written, in loops we must keep it
> + keep_for_full_loop = true;
> + } else {
> + // same thing for switch-case
> + auto fw_switch_scope = scopes[idx].in_switchcase();
> + if (fw_switch_scope >= 0 &&
> scopes[fw_switch_scope].in_loop()) {
> + keep_for_full_loop = true;
> + }
> + }
Simplify this by using an in_conditional() instead of in_ifelse +
in_switchcase().
I was thinking about this, but I also thought that when I want to track
all code path later then I will have to distinguish the two again, but,
on the other hand, adding a method that combines the two is a no-
brainer.
> +
> + /* Only written to, just make sure that renaming
> + * doesn't reuse this register too early (corner
> + * case is the one opcode with two destinations) */
> + if (last_read_scope < 0) {
> + return make_pair(first_write, first_write + 1);
What if there are multiple writes to the temporary?
Good catch! Next text case :)
> + /* propagate lifetime also if there was a continue/break
> + * in a loop and the write was after it (so it constitutes
> + * a conditional write */
It's only conditional if there was a break. Continue doesn't make it
conditional.
I think I made my argument.
+ /* propagate lifetimes before moving to upper scopes */
> + if ((scopes[first_write_scope].type() == sct_loop) &&
> + (keep_for_full_loop || (undefined_read < first_write))) {
> + first_write = scopes[first_write_scope].begin();
> + int lr = scopes[first_write_scope].end();
> + if (last_read < lr)
> + last_read = lr;
> + }
What about:
BGNLOOP
...
BGNLOOP
IF ...
read TEMP[i]
ENDIF
write TEMP[i]
ENDLOOP
...
ENDLOOP
In this case, you don't set keep_for_full_loop, yet the lifetime
must extend for the entire _outer_ loop.
Another test case that needs to be added :)
I see two related problems in the code:
1. In this case, keep_for_full_loop needs to be set to true (because
overwriting first_write in the code above means that the related
check below won't fire).
2. target_level is set to the inner loop even though it really needs
to be set to the outer loop.
Indeed.
Thanks a lot,
Gert
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
