New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
is the precedence of reduce
correct?
#11463
Comments
It might be useful to look at which pattern is more frequently used in code bases to determine which pattern we should prioritize through precedence rules. (rip)grepping op reduce A * B and 0 instances of this pattern: op reduce A - op reduce A
|
reduce
right?reduce
correct?
I ran the following patch through paratest: --- a/compiler/parser/chapel.ypp
+++ b/compiler/parser/chapel.ypp
@@ -359,6 +359,7 @@
%left TNOELSE
%left TELSE
%left TCOMMA
+%left TREDUCE TSCAN TDMAPPED
%left TFOR TFORALL TIF TATOMIC TSYNC TSINGLE
// %left TOWNED TUNMANAGED TSHARED
%left TIN
@@ -376,7 +377,6 @@
%right TUPLUS TUMINUS
%left TSTAR TDIVIDE TMOD
%right TBNOT TBANG
-%left TREDUCE TSCAN TDMAPPED
%right TEXP
%left TCOLON
%left TBORROWED TOWNED TUNMANAGED TSHARED 14 tests failed including futures. Here is a summary:
var a1: [1..3] domain(1) dmapped Block({1..2}); But, this test is just checking what error occurs.
if + reduce Y != numNumbers then
Lots of examples since it's testing the precedence. E.g. var total: int = + reduce A * B;
writeln(+ reduce ([1..3] 1) + [4..6] 2);
writeln(- min reduce (1..3));
AMR code has very many parethesized reductions (suggesting perhaps the other precedence would make the code nicer looking) but the one that changed behavior might be this: return +reduce(signatures(1).array):real / D.numIndices:real;
const execTime = [t in 1..numTrials] max reduce [loc in LocaleSpace] allExecTime(loc)(t);
This is a test of operator precedence.
if (+ reduce A-B) == 11 then writeln("hi"); (test is expecting an error)
e.g. assert(min reduce ARR == -INFINITY);
assert(minloc reduce zip(ARR, ARR.domain) == (-INFINITY, n-1)); |
Looking at the last one - the test that I wrote recently: assert(min reduce ARR == -INFINITY);
assert(minloc reduce zip(ARR, ARR.domain) == (-INFINITY, n-1)); To me, it is mostly a matter of a habit, whether to think of it as |
In the original case where I ran into this, I wanted to do something like
|
Only the following failing test cases cause me some concern with changing the precedence of writeln(- min reduce (1..3)); This one is interesting, because it's fairly unambiguous (since const execTime = [t in 1..numTrials] max reduce [loc in LocaleSpace] allExecTime(loc)(t); I think this is running into the same problem as the above. return +reduce(signatures(1).array):real / D.numIndices:real; Here, the parentheses after + reduce (array:real) / numIndices; Another misleading example would be this: + reduce (A) ** 2; which is interpreted (on master) as So, I think that if we had a goal of supporting this function-invocation-like pattern for reduce, we should handle that by making a specific parser rule for |
The spec rationale claims that the error you get if you misunderstand the precedence is a motivator for the precedence choice. Would we still get a good error if we made the opposite choice? Suppose we chose the opposite precedence and somebody wanted to write max reduce A - min reduce A but meant (max reduce A) - (min reduce A) The compiler would actually interpret it as max reduce (A - min reduce A) Which you might think would compile but have much worse performance (because the inner reduction might be computed once per array element). However that is not what happens today - the sub-expression is evaluated only once. This is an open question in #7904. Hidden below is a benchmark showing that the performance is comparable. Anyway, the situation is that if #7904 is left working the way it works on master, the change in behavior from what was intended is probably harmless. If #7904 changes, it would become a performance problem without any indication from the compiler (Of course it is something we could add a warning specifically about if desired). config const n = 1000000;
config const ntrials = 100000;
use Time;
proc main() {
const A:[1..n] int = 1..n;
var sum = 0;
var t: Timer;
t.start();
for i in 1..ntrials {
sum += (max reduce A) - (min reduce A);
}
t.stop();
writeln("(max reduce A) - (min reduce A) ", t.elapsed(), " s");
t.clear();
t.start();
for i in 1..ntrials {
sum += max reduce (A - min reduce A);
}
t.stop();
writeln("max reduce (A - (min reduce A) ", t.elapsed(), " s");
}
The spec claims that Which tests actually use two or more
Based on some grepping, I think there are many more cases of something like |
There are plenty of decisions we've made in Chapel that have seemed increasingly wrong to me over time (most of which we've now corrected), but the precedence level of reduce and scan has never been one of them, personally. I'm not sure I can explain why any better than the original spec rationale, though. I think of them as being like unary operators; and I think of them as naturally taking an array as an argument, so if I want to reduce or scan an array-like expression, I tend to find that parenthesizing that expression seems natural as well (e.g., |
As a data point, a user recently ran over the pitfall described in this issue: unexpected-result-for-inner-product-using-reduce |
@mppf what do you think about closing this issue? How much room is here for more progress? |
It doesn't sound to me like we're likely to change it but I would still be open to doing so if other people think it is worth changing. However, I'm not sure if this issue got much attention aside from those of us who commented on it. Anyway, I think we could close the issue and say "we're not expecting to change it". |
The spec indicates in section 10.11 that
reduce
has higher precedence than*
. I found this surprising, but I'm not sure I can explain why. (Something about them being multiple tokens makes me think they bind loosely).This issue is just to note the issue in case anybody else runs into it / has run into it and wants to discuss.
For example, if we have
is interpreted as
rather than as
The specification has rationale for it:
The text was updated successfully, but these errors were encountered: