RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On Fri, 22 Mar 2013, Iyer, Balaji V wrote:
Why the random check for a NULL argument? If a NULL argument is valid
(meaning that it makes the code cleaner to allow such arguments rather than
making sure the function isn't called with them), this should be documented
in
the comment above the function; otherwise, if such an argument isn't valid,
there is no need to check for it.
I always tend to check for a null pointer before I access the fields in
the structure. In this case it is unnecessary. In some cases (e.g.
find_rank) there is a good chance a null pointer will be passed into the
function and we need to check that and reject those.
always tend to check for a null pointer is not good practice.
Sometimes such checks are good, and sometimes they are bad.
Each function should have a defined interface that makes clear the
semantics and valid values of its operands - including whether NULL is
valid, and, if so, what the semantics of a NULL argument are. These
semantics should be clear from the comment above the function.
What the semantics should be in each case depends on a global view of the
relevant code. Based on such a global view you need to determine the
interfaces resulting in the cleanest code - in some cases there may be a
special case, the absence of a datastructure as opposed to its presence,
that is naturally represented by a NULL argument, but in that case it's a
judgement to be made for the code as a whole whether this case should be
checked for in the caller or the callee (or maybe further up or down the
call stack in some cases). Where erroneous input to the compiler is the
cause of NULL arguments, passing error_mark_node may be better in some
cases if it means fewer special-case checks.
If in a particular case it shouldn't be possible for a NULL pointer to be
present in a particular place (function argument, variable, etc.), for any
input to the compiler, it is always wrong to have a check for a NULL
pointer that silently continues compilation. Instead, in such cases where
NULL is invalid you have two options:
* No check, if NULL is present and dereferenced the compiler will crash.
* A check inside a gcc_assert, to verify this precondition of the function
and give a slightly friendlier internal compiler error than a segfault.
(Or any other similar option resulting in an abort with an ICE.)
You need to judge in each case which of the two is appropriate, just as
with any other case where there is some precondition for a function that
you might or might not decide to verify with an assertion, depending on
factors such as:
* how complicated such a check is to write;
* how expensive the check is when the compiler runs;
* how likely the check is to find bugs in the compiler;
* how helpful the check's presence is to people reading the source code
and trying to understand what the data may look like at a particular
point.
Note also that the fact that you observe a NULL pointer being passed to a
function and causing a crash there does not of itself mean that adding a
check for NULL is a valid fix. It's only a valid fix if analysis of the
issue shows that it was indeed correct for NULL to be passed to that
function for the given input to the compiler. Sometimes it may turn out
that NULL shouldn't have been passed to that function at all for that
input and that the proper fix is elsewhere in the compiler.
+ if (TREE_CODE (fn) == CALL_EXPR)
+{
+ fn_arg = CALL_EXPR_ARG (fn, 0);
+ if (really_constant_p (fn_arg))
I don't think really_constant_p is what's wanted;
http://software.intel.com/sites/default/files/m/4/e/7/3/1/40297-
Intel_Cilk_plus_lang_spec_2.htm
says The argument shall be an integer constant expression., and such
expressions always appear in the C front end as INTEGER_CST. So you can
just
check for INTEGER_CST.
What about C++? This function is shared by both C and C++.
This patch seems just to be for C. really_constant_p is wrong for C,
since INTEGER_CSTs wrapped with conversions can be used to represent
values folded to constants that aren't integer constant expressions.
Maybe you need a conditional on the language, but I don't know what the
right check for C++ might be in that case.
+void
+find_rank (tree array, bool ignore_builtin_fn, size_t *rank) {
+ tree ii_tree;
+ size_t current_rank = 0, ii = 0;
+ an_reduce_type dummy_type = REDUCE_UNKNOWN;
+ if (!array)
+return;
As before, avoid random checks for NULL parameters unless there is an actual
reason to allow them and the comments document that they are allowed and
what the semantics are in that case. In general, explain what ARRAY is - an
expression?
This check is necessary. Find rank can get a NULL pointer and that must
be caught and rejected.
Then make sure the comment explaining the semantics of ARRAY includes the
case of a NULL pointer, explaining what the
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On Mon, 25 Mar 2013, Aldy Hernandez wrote: I always tend to check for a null pointer before I access the fields in the structure. In this case it is unnecessary. In some cases (e.g. find_rank) there is a good chance a null pointer will be passed into the function and we need to check that and reject those. I think what Joseph is suggesting is that if NULL is not valid, then the caller should check this. But if NULL is valid, then it should be documented in the function comment at the top. The caller should only check it if it's valid in the caller but not the callee. If it's invalid in the caller as well, neither should check (except maybe in an assertion if felt appropriate in a particular case). -- Joseph S. Myers jos...@codesourcery.com
RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
Hello Joseph, Aldy et al.,
Attached please find a patch that will fixed the problem below and
another problem you mentioned in a previous email (I had used
really_constant_p(..) and you mentioned that in C we need to check for
INTEGER_CST).
Please let me know if I have missed anything else that you mentioned.
Thanks,
Balaji V. Iyer.
-Original Message-
From: Joseph Myers [mailto:jos...@codesourcery.com]
Sent: Tuesday, March 26, 2013 1:05 PM
To: Aldy Hernandez
Cc: Iyer, Balaji V; gcc-patches@gcc.gnu.org
Subject: Re: [patch] cilkplus array notation for C (clean, independent
patchset,
take 1)
On Mon, 25 Mar 2013, Aldy Hernandez wrote:
I always tend to check for a null pointer before I access the fields
in the structure. In this case it is unnecessary. In some cases
(e.g. find_rank) there is a good chance a null pointer will be
passed into the function and we need to check that and reject those.
I think what Joseph is suggesting is that if NULL is not valid, then
the caller should check this. But if NULL is valid, then it should be
documented in the function comment at the top.
The caller should only check it if it's valid in the caller but not the
callee. If it's
invalid in the caller as well, neither should check (except maybe in an
assertion if
felt appropriate in a particular case).
FIXED!
--
Joseph S. Myers
jos...@codesourcery.com
diff --git a/gcc/c-family/array-notation-common.c
b/gcc/c-family/array-notation-common.c
index b775225..894c02a 100644
--- a/gcc/c-family/array-notation-common.c
+++ b/gcc/c-family/array-notation-common.c
@@ -152,7 +152,7 @@ extract_sec_implicit_index_arg (location_t location, tree
fn)
if (TREE_CODE (fn) == CALL_EXPR)
{
fn_arg = CALL_EXPR_ARG (fn, 0);
- if (really_constant_p (fn_arg))
+ if (TREE_CODE (fn_arg) == INTEGER_CST)
return_int = int_cst_value (fn_arg);
else
{
diff --git a/gcc/c/c-array-notation.c b/gcc/c/c-array-notation.c
index 9ee281e..dd0a1b0 100755
--- a/gcc/c/c-array-notation.c
+++ b/gcc/c/c-array-notation.c
@@ -52,9 +52,7 @@ struct inv_list
/* Set *RANK of expression ARRAY, ignoring array notation specific built-in
functions if IGNORE_BUILTIN_FN is true. The ORIG_EXPR is printed out if an
error occured in the rank calculation. The functions returns false if it
- encounters an error in rank calculation. If ARRAY can be NULL, since it is
- recursively accessing all the fields in a subtree. If so, then just return
- true.
+ encounters an error in rank calculation.
For example, an array notation of A[:][:] or B[0:10][0:5:2] or C[5][:][1:0]
all have a rank of 2. */
@@ -66,10 +64,8 @@ find_rank (location_t loc, tree orig_expr, tree array, bool
ignore_builtin_fn,
tree ii_tree;
size_t ii = 0, current_rank = 0;
an_reduce_type dummy_type = REDUCE_UNKNOWN;
-
- if (!array)
-return true;
- else if (TREE_CODE (array) == ARRAY_NOTATION_REF)
+
+ if (TREE_CODE (array) == ARRAY_NOTATION_REF)
{
for (ii_tree = array;
ii_tree TREE_CODE (ii_tree) == ARRAY_NOTATION_REF;
@@ -111,7 +107,8 @@ find_rank (location_t loc, tree orig_expr, tree array, bool
ignore_builtin_fn,
}
else
for (ii = 0; ii TREE_CODE_LENGTH (TREE_CODE (array)); ii++)
- if (!find_rank (loc, orig_expr, TREE_OPERAND (array, ii),
+ if (TREE_OPERAND (array, ii)
+ !find_rank (loc, orig_expr, TREE_OPERAND (array, ii),
ignore_builtin_fn, rank))
return false;
}
@@ -133,21 +130,22 @@ extract_array_notation_exprs (tree node, bool
ignore_builtin_fn,
size_t ii = 0;
an_reduce_type dummy_type = REDUCE_UNKNOWN;
- if (!node)
-return;
- else if (TREE_CODE (node) == ARRAY_NOTATION_REF)
+ if (TREE_CODE (node) == ARRAY_NOTATION_REF)
{
vec_safe_push (*array_list, node);
return;
}
else if (TREE_CODE (node) == TREE_LIST)
{
- extract_array_notation_exprs (TREE_PURPOSE (node), ignore_builtin_fn,
- array_list);
- extract_array_notation_exprs (TREE_VALUE (node), ignore_builtin_fn,
- array_list);
- extract_array_notation_exprs (TREE_CHAIN (node), ignore_builtin_fn,
- array_list);
+ if (TREE_PURPOSE (node))
+ extract_array_notation_exprs (TREE_PURPOSE (node), ignore_builtin_fn,
+ array_list);
+ if (TREE_VALUE (node))
+ extract_array_notation_exprs (TREE_VALUE (node), ignore_builtin_fn,
+ array_list);
+ if (TREE_CHAIN (node))
+ extract_array_notation_exprs (TREE_CHAIN (node), ignore_builtin_fn,
+ array_list);
}
else if (TREE_CODE (node) == STATEMENT_LIST)
{
@@ -181,8 +179,9 @@ extract_array_notation_exprs (tree node, bool
ignore_builtin_fn
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On 03/22/13 17:03, Iyer, Balaji V wrote:
I have not fixed all the issues below (the big one that is left is the bultin function
representation that Joseph Pointed out). I have fixed most of the other issues. All the
things I have fixed are marked by FIXED!
Don't worry, I can work on the builtin function representation.
I am keeping a list of pending issues on the wiki
(http://gcc.gnu.org/wiki/cilkplus-merge) with my name in parenthesis for
items I am working on. Particularly, I have added a sub-section for
array notation items that have been pointed out in reviews but have not
been completed. I suggest you keep this list up to date as well, so we
don't loose track of what has been pointed out.
diff --git a/gcc/c-family/ChangeLog.cilkplus b/gcc/c-family/ChangeLog.cilkplus
index 6591fd1..10db29b 100644
--- a/gcc/c-family/ChangeLog.cilkplus
+++ b/gcc/c-family/ChangeLog.cilkplus
@@ -1,7 +1,11 @@
+2013-03-22 Balaji V. Iyer balaji.v.i...@intel.com
+
+ * c-pretty-print.c (pp_c_expression): Added ARRAY_NOTATION_REF case.
+
2013-03-20 Balaji V. Iyer balaji.v.i...@intel.com
* c-common.c (c_define_builtins): When cilkplus is enabled, the
You can combine changelog entries into one entry. This will make it
easier when we merge into mainline. So basically, add the
c-pretty-print.c entry to the entry below it.
Non-static function declarations like this should not be inside a .c file.
If these functions are used outside this file, there should be an associated
header that declares them; include it in the .c file. If only used inside the
.c file
that defines them, make them static (and topologically sort static functions
inside a source file so that forward static declarations are only needed for
cases
of recursion).
+/* Mark the FNDECL as cold, meaning that the function specified by FNDECL
is
+ not run as is. */
The cold attribute means unlikely to be executed rather than not run as is.
Maybe not run as is is what's relevant here, but I'm not clear why this
attribute
would be useful for built-in functions at all - the documentation suggests it's
only relevant when a user defines a function themselves, and affects the code
generated for that function, so wouldn't be relevant at all for built-in
functions.
I see you fixed this. Since you are only fixing some of the items
Joseph pointed out in this patch, please put FIXED below each item you
did to aid in reviewing.
+void
+array_notation_init_builtins (void)
Other built-in functions use various .def files (builtins.def and the files it
includes)
to avoid lots of repetitive code like this - can you integrate this with that
mechanism? If you do so, then you should be able to avoid (or massively
simplify) functions like:
+/* Returns true if the function call specified in FUNC_NAME is
+ __sec_implicit_index. */
+
+bool
+is_sec_implicit_index_fn (tree func_name)
because code can use the BUILT_IN_* enum values to test whether a particular
function is in use - which is certainly cleaner than using strcmp against the
function name.
And here put FIXED if fixed, or Aldy is going to work on this or
remove it altogether so it's not assumed that it was fixed by this patch
since you're quoting it.
+/* Returns the first and only argument for FN, which should be a
+ sec_implicit_index function. FN's location in the source file is is
+ indicated by LOCATION. */
+
+int
+extract_sec_implicit_index_arg (location_t location, tree fn) {
+ tree fn_arg;
+ HOST_WIDE_INT return_int = 0;
+ if (!fn)
+return -1;
Why the random check for a NULL argument? If a NULL argument is valid
(meaning that it makes the code cleaner to allow such arguments rather than
making sure the function isn't called with them), this should be documented in
the comment above the function; otherwise, if such an argument isn't valid,
there is no need to check for it.
I always tend to check for a null pointer before I access the fields in the
structure. In this case it is unnecessary. In some cases (e.g. find_rank) there
is a good chance a null pointer will be passed into the function and we need to
check that and reject those.
I think what Joseph is suggesting is that if NULL is not valid, then the
caller should check this. But if NULL is valid, then it should be
documented in the function comment at the top.
+ if (TREE_CODE (fn) == CALL_EXPR)
+{
+ fn_arg = CALL_EXPR_ARG (fn, 0);
+ if (really_constant_p (fn_arg))
I don't think really_constant_p is what's wanted;
http://software.intel.com/sites/default/files/m/4/e/7/3/1/40297-
Intel_Cilk_plus_lang_spec_2.htm
says The argument shall be an integer constant expression., and such
expressions always appear in the C front end as INTEGER_CST. So you can just
check for INTEGER_CST.
What about C++? This function is shared by both C and C++.
Same thing for C++, but...
Now a subtlety here is that the function argument will have been folded by
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
The specification doesn't seem very clear on to what extent the __sec_*
operations must act like functions (what happens if someone puts parentheses
around the __sec_* name, for example - that wouldn't work with the keyword
approach). So the specification should be clarified there, but I think saying
the
__sec_* operations are syntactically special, like keywords, is more appropriate
than requiring other uses to work.
+ return_int = (int) int_cst_value (fn_arg);
+ else
+ {
+ if (location == UNKNOWN_LOCATION EXPR_HAS_LOCATION (fn))
+ location = EXPR_LOCATION (fn);
+ error_at (location, __sec_implicit_index parameter must be a
+ constant integer expression);
The term is integer constant expression not constant integer expression.
FIXED!
...it looks like you're going to have to rework all this as a keyword.
OK, CAN I LOOK AT THIS AFTER WE FINISH THE BUILTIN FUNCTION IMPLEMENTATION FIX?
Yes.
Thank you for fixing everything I pointed out. Let's now wait for
Joseph to give the final ok. There are some things he suggested, that I
didn't look at at all, so I am deferring to him.
Thanks again.
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On Mar 21, 2013, at 4:33 PM, Aldy Hernandez al...@redhat.com wrote: I can look into this later on, but this problem happened even when I replaced cilkplus' compile.exp, errors.exp, and execute.exp into just an exit. So it seems unrelated to the cilk patch set. Ah, I withdraw my objection. The bug can't be in those file, if those files say exit and the problem persists. Then, we're into conflation of filenames if I had to guess, which I hate doing, but, if it proves accurate, then changing the spelling is exactly the right fix, or, at least, it isn't unreasonable. Thanks.
RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
From: gcc-patches-ow...@gcc.gnu.org [gcc-patches-ow...@gcc.gnu.org] on behalf of Mike Stump [m...@mrs.kithrup.com] Sent: Friday, March 22, 2013 6:36 PM To: Aldy Hernandez Cc: Jakub Jelinek; Jeff Law; Joseph S. Myers; Iyer, Balaji V; gcc-patches Subject: Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1) On Mar 21, 2013, at 4:33 PM, Aldy Hernandez al...@redhat.com wrote: I can look into this later on, but this problem happened even when I replaced cilkplus' compile.exp, errors.exp, and execute.exp into just an exit. So it seems unrelated to the cilk patch set. Ah, I withdraw my objection. The bug can't be in those file, if those files say exit and the problem persists. Then, we're into conflation of filenames if I had to guess, which I hate doing, but, if it proves accurate, then changing the spelling is exactly the right fix, or, at least, it isn't unreasonable. Thanks. Hi Mike, I can confirm that renaming scripts to something unique fixed the issue. Thanks, Balaji V. Iyer.
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On Mar 22, 2013, at 6:36 PM, Iyer, Balaji V balaji.v.i...@intel.com wrote: I can confirm that renaming scripts to something unique fixed the issue. That's what others have said, I've not see it first hand.
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
All these builtins need to be documented in doc/.
DONE!
+initialize builtin functions are stored in @file{array-notation-common.c}. In
+the current array notation implementation there are 12 builtin reduction
+operations. Details about these functions and their usage are available in
+the Cilk Plus language specification at @w{@uref{http://www.cilkplus.org}}.
+
+@itemize @bullet
+@item __sec_reduce_add
+@item __sec_reduce_mul
+@item __sec_reduce_max
+@item __sec_reduce_min
+@item __sec_reduce_max_ind
First, the common documentation idiom is built-in not builtin.
Second, these should be documented in extend.texi along with the rest of
the builtins listed there.
As I'd mentioned, you have .exp files named compile.exp and execute.exp which
seem to be causing ambiguity problems in parallel checks (make check -jN). For
some reason, with this patch, the rest of dg.exp fails to run after Cilkplus'
compile/execute.exp runs. Renaming these to something less generic does the
trick. Do you mind prefixing all the .exp's with cilkplus_ or something
similar?
FIXED! I added the cilkplus_AN_c_ prefix to everything, where an stands for
Array Notation . This way it won't interfere with the future Cilk Plus patches' test case.
I think eventually all the compile tests for all of cilkplus should go
in one directory and one for the execute tests. There is no need for a
separate directory for the array notation, etc, etc. For that matter,
we should probably munge everything into one dg style directory. But
this is ok for now.
--- a/gcc/testsuite/gcc.dg/cilk-plus/array_notation/compile/array_test2.c
+++ b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/compile/array_test2.c
@@ -26,7 +26,7 @@ int main(int argc, char **argv)
array[ii] = 10;
array2[ii] = 500;
}
- array2[0:10:2] = array[0:10:2];
+ array2[0:5:2] = array[0:5:2];
What is this non-related change?
If these are mere syntax error tests, I suggest you get rid of all the
optimization
variants. Surely there is no need to test the error at -O, then at -O2, etc.
-O0
should suffice. For that matter, you shouldn't pass any optimization flag and
let
the test itself set the flags with // dg-options or whatever in the test
itself (if
for some reason you have a test that has one particular error only reportable at
some optimization level).
I did this purely as a safety measure for the commonly used flags that I know
of. If it won't cause too much of a problem I would like to keep it.
All these errors are in the front-end, so optimization shouldn't matter.
It just seems like over kill to run 750 tests for just 15 individual
.c files. We can leave this for now.
And please make sure there are no regressions on the branch when checking
with make check -k -jN (N 1) and for a plain serial check-- at least while
we
iron out this dejagnu setup.
It works for make -j8 check on my 8 core machine.
I assume make -j1 still works and has no regressions?
Can you repost an updated (and tested) patch?
Thanks.
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On 03/21/13 01:09, Jakub Jelinek wrote: On Wed, Mar 20, 2013 at 11:30:58PM -0600, Jeff Law wrote: On 03/20/2013 10:33 AM, Aldy Hernandez wrote: As I'd mentioned, you have .exp files named compile.exp and execute.exp which seem to be causing ambiguity problems in parallel checks (make check -jN). For some reason, with this patch, the rest of dg.exp fails to run after Cilkplus' compile/execute.exp runs. Renaming these to something less generic does the trick. Do you mind prefixing all the .exp's with cilkplus_ or something similar? Renaming is desirable anyway, people who run make check-gcc execute.exp=something don't expect to run also some subset of cilk+ tests. The Makefile runs some execute.exp=2* and similar when parallelized, see check_gcc_parallelize in gcc/Makefile.in. Right, that's exactly how I found the problem :). Anyway, have you tested that without parallelization make check doesn't skip some tests? Often when a new *.exp file say sets some globals and never resets them, this could affect following *.exp files. Balaji, please check the corresponding .sum files before and after your patch to make sure that the same number of tests are being tested. We have a nifty script in contrib/compare_tests for this task. And as Jakub has said, check (with and) without parallelization.
RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
Balaji, please check the corresponding .sum files before and after your patch to make sure that the same number of tests are being tested. We have a nifty script in contrib/compare_tests for this task. That's how I verify it. (I grep for the ^FAIL in trunk and the applied branch and make sure the output files are the same by going through it). Did I miss anything? And as Jakub has said, check (with and) without parallelization. Yes, I am doing that also for the patch I am submitting.
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On 03/21/13 08:06, Iyer, Balaji V wrote: Balaji, please check the corresponding .sum files before and after your patch to make sure that the same number of tests are being tested. We have a nifty script in contrib/compare_tests for this task. That's how I verify it. (I grep for the ^FAIL in trunk and the applied branch and make sure the output files are the same by going through it). Did I miss anything? If you're using compare_tests, you should be fine. But just grepping for FAIL won't do because there are tests that could have passed before, but are no longer being tested, so they don't show up as a fail. I believe compare_tests complains with tests that used to pass but have disappeared (or something similar). And as Jakub has said, check (with and) without parallelization. Yes, I am doing that also for the patch I am submitting. Thank you.
RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
-Original Message- From: Aldy Hernandez [mailto:al...@redhat.com] Sent: Thursday, March 21, 2013 9:09 AM To: Iyer, Balaji V Cc: Jakub Jelinek; Jeff Law; Joseph S. Myers; gcc-patches Subject: Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1) On 03/21/13 08:06, Iyer, Balaji V wrote: Balaji, please check the corresponding .sum files before and after your patch to make sure that the same number of tests are being tested. We have a nifty script in contrib/compare_tests for this task. That's how I verify it. (I grep for the ^FAIL in trunk and the applied branch and make sure the output files are the same by going through it). Did I miss anything? If you're using compare_tests, you should be fine. But just grepping for FAIL won't do because there are tests that could have passed before, but are no longer being tested, so they don't show up as a fail. I believe compare_tests complains with tests that used to pass but have disappeared (or something similar). I first look at the expected passes, expected fails, etc. If those numbers match up, then I do what I said above . Otherwise I look at things that have failed that shouldn't and/or passed that shouldn't have (this, should almost never happen because all the Cilk plus related code are all enclosed between inside an if (flag_enable_cilkplus) statement). And as Jakub has said, check (with and) without parallelization. Yes, I am doing that also for the patch I am submitting. Thank you.
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
I have found some little nits that I will point out in a reply to this message. Balaji, in Joseph's last review he mentioned: In find_rank you have error (Rank Mismatch!); - this is not a properly formatted error message according to the GNU Coding standards (which typically would not have any uppercase). I'd also suggest that when you find a rank, you store (through a location_t * pointer) the location of the first expression found with that rank, so if you then find a mismatching rank you can use error_at to point to that rank and then inform to point to the previous rank it didn't match. I see you have dispensed with the rank mismatch error altogether. Was this on purpose? For example, you now have: for (ii_tree = array; ii_tree TREE_CODE (ii_tree) == ARRAY_NOTATION_REF; ii_tree = ARRAY_NOTATION_ARRAY (ii_tree)) current_rank++; if (*rank == 0) *rank = current_rank; Which is basically failing to set *rank when it's already set (with no error). Is this on purpose? If so, can you explain? If it's on purpose, document it in the comment at the top of the function. And then, why don't you exit the function immediately upon entry if *rank is non-zero? It's a waste of time to do the rest of the analysis if you're just going to throw it away. Furthermore, Joseph suggested you store the location of the initial rank so you can give a meaningful error message later and tell the user where the mismatch is occurring and where the original rank occurred. Aldy
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On Wed, 20 Mar 2013, Aldy Hernandez wrote:
Joseph, folks, et al... How does this look?
This review largely deals with coding style (interpreted broadly). I'll
review more of the substance separately later; reposting with fixes for
all the accumulated issues is probably a good idea anyway, to avoid the
same issues coming up repeatedly.
* c-common.c (c_define_builtins): When cilkplus is enabled, the
function array_notation_init_builtins() is called.
Don't use () after a function name when referring to the function.
diff --git a/gcc/c-family/array-notation-common.c
b/gcc/c-family/array-notation-common.c
+int extract_sec_implicit_index_arg (location_t, tree);
+bool is_sec_implicit_index_fn (tree);
+void array_notation_init_builtins (void);
Non-static function declarations like this should not be inside a .c file.
If these functions are used outside this file, there should be an
associated header that declares them; include it in the .c file. If only
used inside the .c file that defines them, make them static (and
topologically sort static functions inside a source file so that forward
static declarations are only needed for cases of recursion).
+/* Mark the FNDECL as cold, meaning that the function specified by FNDECL is
+ not run as is. */
The cold attribute means unlikely to be executed rather than not run as
is. Maybe not run as is is what's relevant here, but I'm not clear
why this attribute would be useful for built-in functions at all - the
documentation suggests it's only relevant when a user defines a function
themselves, and affects the code generated for that function, so wouldn't
be relevant at all for built-in functions.
+void
+array_notation_init_builtins (void)
Other built-in functions use various .def files (builtins.def and the
files it includes) to avoid lots of repetitive code like this - can you
integrate this with that mechanism? If you do so, then you should be able
to avoid (or massively simplify) functions like:
+/* Returns true if the function call specified in FUNC_NAME is
+ __sec_implicit_index. */
+
+bool
+is_sec_implicit_index_fn (tree func_name)
because code can use the BUILT_IN_* enum values to test whether a
particular function is in use - which is certainly cleaner than using
strcmp against the function name.
+/* Returns the first and only argument for FN, which should be a
+ sec_implicit_index function. FN's location in the source file is is
+ indicated by LOCATION. */
+
+int
+extract_sec_implicit_index_arg (location_t location, tree fn)
+{
+ tree fn_arg;
+ HOST_WIDE_INT return_int = 0;
+ if (!fn)
+return -1;
Why the random check for a NULL argument? If a NULL argument is valid
(meaning that it makes the code cleaner to allow such arguments rather
than making sure the function isn't called with them), this should be
documented in the comment above the function; otherwise, if such an
argument isn't valid, there is no need to check for it.
You declare return_int as HOST_WIDE_INT, but it only receives a value cast
to int, and is used only to store a value returned as int. Either use int
consistently, or HOST_WIDE_INT consistently, but I don't see a reason to
use both.
+ if (TREE_CODE (fn) == CALL_EXPR)
+{
+ fn_arg = CALL_EXPR_ARG (fn, 0);
+ if (really_constant_p (fn_arg))
I don't think really_constant_p is what's wanted;
http://software.intel.com/sites/default/files/m/4/e/7/3/1/40297-Intel_Cilk_plus_lang_spec_2.htm
says The argument shall be an integer constant expression., and such
expressions always appear in the C front end as INTEGER_CST. So you can
just check for INTEGER_CST.
Now a subtlety here is that the function argument will have been folded by
this point, meaning that cases that aren't integer constant expressions in
C standard terms will be wrongly allowed (both by the original code and by
a version checking against INTEGER_CST). In such cases, the way to get
things checked correctly is to use a keyword rather than a built-in
function - as with __builtin_choose_expr or __builtin_shuffle, for
example. Since this operation seems special in ways that built-in
functions generally aren't, that seems reasonable anyway. So the code
parsing this keyword would check that the argument is an INTEGER_CST, of
integer type (since INTEGER_CSTs can have pointer type in GCC), like that
for __builtin_choose_expr does. It would then quite likely create its own
tree code for the operation, rather than using a CALL_EXPR at all. (It
would need to manage converting to int, given how the specification
defines things in terms of a prototype for type int - so e.g. a constant
1ULL 32 would act like 0 if int is 32 bits, under the present
specification.)
The specification doesn't seem very clear on to what extent the __sec_*
operations must act like functions (what happens if someone puts
parentheses around the __sec_* name, for example -
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
Continuing the review for coding style...
diff --git a/gcc/c/c-parser.c b/gcc/c/c-parser.c
+extern bool contains_array_notation_expr (tree);
+extern struct c_expr fix_array_notation_expr (location_t, enum tree_code,
+ struct c_expr);
+extern tree fix_conditional_array_notations (tree);
+extern tree expand_array_notation_exprs (tree);
Again, include appropriate headers; no extern function declarations like
this in .c files.
+ error_at (c_parser_peek_token (parser)-location,
+ array notations cannot be used in declaration.);
No . at end of diagnostic.
+ error_at (c_parser_peek_token (parser)-location,
+ array notations cannot be used in declaration.);
Likewise.
+ error_at (loc, array notations cannot be used in a
+ condition for a for-loop.);
Likewise.
+static tree
+c_parser_array_notation (location_t loc, c_parser *parser, tree
initial_index,
+ tree array_value)
+{
+ c_token *token = NULL;
+ tree start_index = NULL_TREE, end_index = NULL_TREE, stride = NULL_TREE;
+ tree value_tree = NULL_TREE, type = NULL_TREE, array_type = NULL_TREE;
+ tree array_type_domain = NULL_TREE;
+ double_int x;
+
+ if (!array_value || array_value == error_mark_node)
Can NULL actually occur here?
+ token = c_parser_peek_token (parser);
+
+ if (token == NULL)
c_parser_peek_token can never return NULL (EOF is a CPP_EOF token).
+ error_at (loc, start-index and length fields necessary for
+ using array notations in pointers.);
No . at end of diagnostic.
+ error_at (loc, array notations cannot be used with function
+ type.);
Likewise.
+ error_at (loc, array notations cannot be used with
+ function pointer arrays.);
Likewise.
+ error_at (loc, start-index and length fields necessary for
+ using array notations in dimensionless arrays.);
Likewise.
+ error_at (loc, start-index and length fields necessary for
+ using array notations in variable-length arrays.);
Likewise.
+ c_parser_skip_until_found (parser, CPP_CLOSE_SQUARE, NULL);
+ return error_mark_node;
+ }
+ x = TREE_INT_CST (TYPE_MAXVAL (array_type_domain));
+ x.low++;
If you want to increment a double_int value, use operator ++ on the
double_int itself; don't just change the low part and ignore possible
overflow.
+ error_at (loc, array notations cannot be used with function
+ type.);
No . at end of diagnostic.
+ error_at (loc, array notations cannot be used with
+ function pointer arrays.);
Likewise.
diff --git a/gcc/c/c-typeck.c b/gcc/c/c-typeck.c
index ddb6d39..15dc83d 100644
--- a/gcc/c/c-typeck.c
+++ b/gcc/c/c-typeck.c
@@ -103,6 +103,8 @@ static void readonly_warning (tree, enum lvalue_use);
static int lvalue_or_else (location_t, const_tree, enum lvalue_use);
static void record_maybe_used_decl (tree);
static int comptypes_internal (const_tree, const_tree, bool *, bool *);
+extern bool contains_array_notation_expr (tree);
+extern tree find_correct_array_notation_type (tree);
Include headers, don't add extern function declarations in .c files.
@@ -2303,7 +2305,17 @@ build_array_ref (location_t loc, tree array, tree
index)
if (TREE_TYPE (array) == error_mark_node
|| TREE_TYPE (index) == error_mark_node)
return error_mark_node;
-
+
Don't change a blank line by adding trailing whitespace to it.
+ error_at (loc, rank of the array's index is greater than 1.);
No . at end of diagnostic.
+ if (flag_enable_cilkplus name
+!strncmp (IDENTIFIER_POINTER (name), __sec_reduce, 12))
Should replace by something cleaner than examining the name, once the way
these built-ins are implemented has been changed to have enum values.
+ if (flag_enable_cilkplus fundecl DECL_NAME (fundecl)
+!strncmp (IDENTIFIER_POINTER (DECL_NAME (fundecl)),
+__sec_reduce, 12))
Likewise.
+ /* If array notation is used and Cilk Plus is enabled, then we do not
+ worry about this error now. We will handle them in a later place. */
+ if (flag_enable_cilkplus DECL_NAME (fundecl)
+!strncmp (IDENTIFIER_POINTER (DECL_NAME (fundecl)), __sec_reduce,
+12))
Likewise.
@@ -5097,7 +5130,6 @@ convert_for_assignment (location_t location, tree type,
tree rhs,
enum tree_code coder;
tree rname = NULL_TREE;
bool objc_ok = false;
-
if (errtype == ic_argpass)
{
tree selector;
Avoid spurious changes like this not related to the substance of the
patch.
+ if (flag_enable_cilkplus contains_array_notation_expr (cond))
+{
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On Mar 20, 2013, at 11:09 PM, Jakub Jelinek ja...@redhat.com wrote: On Wed, Mar 20, 2013 at 11:30:58PM -0600, Jeff Law wrote: On 03/20/2013 10:33 AM, Aldy Hernandez wrote: As I'd mentioned, you have .exp files named compile.exp and execute.exp which seem to be causing ambiguity problems in parallel checks (make check -jN). For some reason, with this patch, the rest of dg.exp fails to run after Cilkplus' compile/execute.exp runs. Renaming these to something less generic does the trick. Do you mind prefixing all the .exp's with cilkplus_ or something similar? Renaming is desirable anyway, So, the problem that causes it to not work needs to be fixed, before the patches go in. Renaming is a form of bug pushing and that can't be used to fix the problem. When the underlying problem is fixed, then rename, not before. The problem is that the entire sanity of the test suite depends critically on resetting the environment to be back the way it was upon the finishing of any particular .exp. You can run with -v -v -v -v and see check differences in the output, it might let you narrow down the issue. You can run env (or parry env) before and after, and quickly see if it is due to an environment variable. In theory, you can have tcl dump all the variables from the symbol tables, (info locals and info globals being the starting point)… but you'd need to differentiate between the variables that must be the same, and the variables that can be different. One might be able to turn on tracing to find it, though, that can make your eyes bleed.
RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
Please see my response below: -Original Message- From: Aldy Hernandez [mailto:al...@redhat.com] Sent: Thursday, March 21, 2013 10:25 AM To: Joseph S. Myers Cc: Iyer, Balaji V; gcc-patches Subject: Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1) I have found some little nits that I will point out in a reply to this message. Balaji, in Joseph's last review he mentioned: In find_rank you have error (Rank Mismatch!); - this is not a properly formatted error message according to the GNU Coding standards (which typically would not have any uppercase). I'd also suggest that when you find a rank, you store (through a location_t * pointer) the location of the first expression found with that rank, so if you then find a mismatching rank you can use error_at to point to that rank and then inform to point to the previous rank it didn't match. I see you have dispensed with the rank mismatch error altogether. Was this on purpose? For example, you now have: for (ii_tree = array; ii_tree TREE_CODE (ii_tree) == ARRAY_NOTATION_REF; ii_tree = ARRAY_NOTATION_ARRAY (ii_tree)) current_rank++; if (*rank == 0) *rank = current_rank; Which is basically failing to set *rank when it's already set (with no error). Is this on purpose? If so, can you explain? If it's on purpose, document it in the comment at the top of the function. And then, why don't you exit the function immediately upon entry if *rank is non- zero? It's a waste of time to do the rest of the analysis if you're just going to throw it away. Furthermore, Joseph suggested you store the location of the initial rank so you can give a meaningful error message later and tell the user where the mismatch is occurring and where the original rank occurred. I believe I have done what Joseph mentioned. If you would look at line 473 c-array-notation.c it mentions when LHS is scalar while RHS is not (which is unacceptable). Also, if they both have array notations and there is a rank mismatch, then it is pointed out in line 490 of c-array-notation.c. Aldy
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On 03/21/13 14:07, Iyer, Balaji V wrote: Please see my response below: -Original Message- From: Aldy Hernandez [mailto:al...@redhat.com] Sent: Thursday, March 21, 2013 10:25 AM To: Joseph S. Myers Cc: Iyer, Balaji V; gcc-patches Subject: Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1) I have found some little nits that I will point out in a reply to this message. Balaji, in Joseph's last review he mentioned: In find_rank you have error (Rank Mismatch!); - this is not a properly formatted error message according to the GNU Coding standards (which typically would not have any uppercase). I'd also suggest that when you find a rank, you store (through a location_t * pointer) the location of the first expression found with that rank, so if you then find a mismatching rank you can use error_at to point to that rank and then inform to point to the previous rank it didn't match. I see you have dispensed with the rank mismatch error altogether. Was this on purpose? For example, you now have: for (ii_tree = array; ii_tree TREE_CODE (ii_tree) == ARRAY_NOTATION_REF; ii_tree = ARRAY_NOTATION_ARRAY (ii_tree)) current_rank++; if (*rank == 0) *rank = current_rank; Which is basically failing to set *rank when it's already set (with no error). Is this on purpose? If so, can you explain? If it's on purpose, document it in the comment at the top of the function. And then, why don't you exit the function immediately upon entry if *rank is non- zero? It's a waste of time to do the rest of the analysis if you're just going to throw it away. Furthermore, Joseph suggested you store the location of the initial rank so you can give a meaningful error message later and tell the user where the mismatch is occurring and where the original rank occurred. I believe I have done what Joseph mentioned. If you would look at line 473 c-array-notation.c it mentions when LHS is scalar while RHS is not (which is unacceptable). Also, if they both have array notations and there is a rank mismatch, then it is pointed out in line 490 of c-array-notation.c. I see. You have moved the diagnostic to the caller. However, you still have not saved the location_t for giving a better diagnostic. Don't worry. I'll put this in a laundry list I'm going to keep in the wiki so we don't loose track of things. I'll take this one. Also, if you're only setting *rank if it's previously 0, why not exit if *rank==0 upon entry? Aldy
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On 03/21/13 11:54, Mike Stump wrote: On Mar 20, 2013, at 11:09 PM, Jakub Jelinek ja...@redhat.com wrote: On Wed, Mar 20, 2013 at 11:30:58PM -0600, Jeff Law wrote: On 03/20/2013 10:33 AM, Aldy Hernandez wrote: As I'd mentioned, you have .exp files named compile.exp and execute.exp which seem to be causing ambiguity problems in parallel checks (make check -jN). For some reason, with this patch, the rest of dg.exp fails to run after Cilkplus' compile/execute.exp runs. Renaming these to something less generic does the trick. Do you mind prefixing all the .exp's with cilkplus_ or something similar? Renaming is desirable anyway, So, the problem that causes it to not work needs to be fixed, before the patches go in. Renaming is a form of bug pushing and that can't be used to fix the problem. When the underlying problem is fixed, then rename, not before. The problem is that the entire sanity of the test suite depends critically on resetting the environment to be back the way it was upon the finishing of any particular .exp. You can run with -v -v -v -v and see check differences in the output, it might let you narrow down the issue. You can run env (or parry env) before and after, and quickly see if it is due to an environment variable. In theory, you can have tcl dump all the variables from the symbol tables, (info locals and info globals being the starting point)… but you'd need to differentiate between the variables that must be the same, and the variables that can be different. One might be able to turn on tracing to find it, though, that can make your eyes bleed. I can look into this later on, but this problem happened even when I replaced cilkplus' compile.exp, errors.exp, and execute.exp into just an exit. So it seems unrelated to the cilk patchset.
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On 03/20/13 10:30, Aldy Hernandez wrote:
I have found some little nits that I will point out in a reply to this
message.
Joseph, folks, et al... How does this look?
Thanks.
Balaji:
+void
+array_notation_init_builtins (void)
+{
+ tree func_type = NULL_TREE;
+ tree new_func = NULL_TREE;
+ func_type = build_function_type_list (integer_type_node, ptr_type_node,
+ NULL_TREE);
+ new_func = build_fn_decl (__sec_reduce_add, func_type);
+ mark_cold (new_func);
+ new_func = lang_hooks.decls.pushdecl (new_func);
etc
etc
All these builtins need to be documented in doc/.
+load_lib gcc-dg.exp
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -fcilkplus
+dg-finish
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -O0 -fcilkplus
+dg-finish
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -O1 -fcilkplus
+dg-finish
etc
etc
Can't you do all these dg-runtest's within one pair of
dg-init/dg-finish? Similarly for the other .exp files.
As I'd mentioned, you have .exp files named compile.exp and execute.exp
which seem to be causing ambiguity problems in parallel checks (make
check -jN). For some reason, with this patch, the rest of dg.exp fails
to run after Cilkplus' compile/execute.exp runs. Renaming these to
something less generic does the trick. Do you mind prefixing all the
.exp's with cilkplus_ or something similar?
[Perhaps someone can pontificate as to what the actual problem is here
and describe it. Dejagnu is a mystery to me.]
diff --git a/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/errors.exp
b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/errors.exp
new file mode 100644
index 000..6d7604b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/errors.exp
@@ -0,0 +1,65 @@
+# Copyright (C) 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or modify
+# it under the terms of the GNU General Public License as published by
+# the Free Software Foundation; either version 3 of the License, or
+# (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful,
+# but WITHOUT ANY WARRANTY; without even the implied warranty of
+# MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the
+# GNU General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License
+# along with GCC; see the file COPYING3. If not see
+# http://www.gnu.org/licenses/.
+
+# Written by Balaji V. Iyer balaji.v.i...@intel.com
+
+
+load_lib gcc-dg.exp
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -fcilkplus
+dg-finish
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -O0 -fcilkplus
+dg-finish
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -O1 -fcilkplus
+dg-finish
If these are mere syntax error tests, I suggest you get rid of all the
optimization variants. Surely there is no need to test the error at -O,
then at -O2, etc. -O0 should suffice. For that matter, you shouldn't
pass any optimization flag and let the test itself set the flags with
// dg-options or whatever in the test itself (if for some reason you
have a test that has one particular error only reportable at some
optimization level).
And please make sure there are no regressions on the branch when
checking with make check -k -jN (N 1) and for a plain serial check--
at least while we iron out this dejagnu setup.
Thanks.
RE: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
HI Aldy, Joseph et al.,
Attached, please find a fixed patch. Please see my responses to Aldy's
questions below:
-Original Message-
From: Aldy Hernandez [mailto:al...@redhat.com]
Sent: Wednesday, March 20, 2013 12:33 PM
To: Joseph S. Myers
Cc: Iyer, Balaji V; gcc-patches
Subject: Re: [patch] cilkplus array notation for C (clean, independent
patchset,
take 1)
On 03/20/13 10:30, Aldy Hernandez wrote:
I have found some little nits that I will point out in a reply to this
message.
Joseph, folks, et al... How does this look?
Thanks.
Balaji:
+void
+array_notation_init_builtins (void)
+{
+ tree func_type = NULL_TREE;
+ tree new_func = NULL_TREE;
+ func_type = build_function_type_list (integer_type_node, ptr_type_node,
+ NULL_TREE);
+ new_func = build_fn_decl (__sec_reduce_add, func_type);
+ mark_cold (new_func);
+ new_func = lang_hooks.decls.pushdecl (new_func);
etc
etc
All these builtins need to be documented in doc/.
DONE!
+load_lib gcc-dg.exp
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -fcilkplus
+dg-finish
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -O0
-fcilkplus
+dg-finish
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -O1
-fcilkplus
+dg-finish
etc
etc
Can't you do all these dg-runtest's within one pair of dg-init/dg-finish?
Similarly
for the other .exp files.
The reason why I did these is so that we can easily isolate the errors to one
run if necessary. I fixed them as you requested.
As I'd mentioned, you have .exp files named compile.exp and execute.exp which
seem to be causing ambiguity problems in parallel checks (make check -jN).
For
some reason, with this patch, the rest of dg.exp fails to run after Cilkplus'
compile/execute.exp runs. Renaming these to something less generic does the
trick. Do you mind prefixing all the .exp's with cilkplus_ or something
similar?
FIXED! I added the cilkplus_AN_c_ prefix to everything, where an stands for
Array Notation . This way it won't interfere with the future Cilk Plus patches'
test case.
[Perhaps someone can pontificate as to what the actual problem is here and
describe it. Dejagnu is a mystery to me.]
diff --git
a/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/errors.exp
b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/errors.exp
new file mode 100644
index 000..6d7604b
--- /dev/null
+++ b/gcc/testsuite/gcc.dg/cilk-plus/array_notation/errors/errors.exp
@@ -0,0 +1,65 @@
+# Copyright (C) 2013 Free Software Foundation, Inc.
+
+# This program is free software; you can redistribute it and/or
+modify # it under the terms of the GNU General Public License as
+published by # the Free Software Foundation; either version 3 of the
+License, or # (at your option) any later version.
+#
+# This program is distributed in the hope that it will be useful, #
+but WITHOUT ANY WARRANTY; without even the implied warranty of #
+MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the # GNU
+General Public License for more details.
+#
+# You should have received a copy of the GNU General Public License #
+along with GCC; see the file COPYING3. If not see #
+http://www.gnu.org/licenses/.
+
+# Written by Balaji V. Iyer balaji.v.i...@intel.com
+
+
+load_lib gcc-dg.exp
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -fcilkplus
+dg-finish
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -O0
-fcilkplus
+dg-finish
+
+dg-init
+dg-runtest [lsort [glob -nocomplain $srcdir/$subdir/*.c]] -O1
-fcilkplus
+dg-finish
If these are mere syntax error tests, I suggest you get rid of all the
optimization
variants. Surely there is no need to test the error at -O, then at -O2, etc.
-O0
should suffice. For that matter, you shouldn't pass any optimization flag
and let
the test itself set the flags with // dg-options or whatever in the test
itself (if
for some reason you have a test that has one particular error only reportable
at
some optimization level).
I did this purely as a safety measure for the commonly used flags that I know
of. If it won't cause too much of a problem I would like to keep it.
And please make sure there are no regressions on the branch when checking
with make check -k -jN (N 1) and for a plain serial check-- at least
while we
iron out this dejagnu setup.
It works for make -j8 check on my 8 core machine.
Thanks,
Balaji V. Iyer.
diff --git a/gcc/doc/passes.texi b/gcc/doc/passes.texi
index 045f964..81b6502 100644
--- a/gcc/doc/passes.texi
+++ b/gcc/doc/passes.texi
@@ -125,7 +125,25 @@ inside conditions, they are transformed using the function
@code{fix_conditional_array_notations}. The C language-specific routines are
located
Re: [patch] cilkplus array notation for C (clean, independent patchset, take 1)
On 03/20/2013 10:33 AM, Aldy Hernandez wrote: As I'd mentioned, you have .exp files named compile.exp and execute.exp which seem to be causing ambiguity problems in parallel checks (make check -jN). For some reason, with this patch, the rest of dg.exp fails to run after Cilkplus' compile/execute.exp runs. Renaming these to something less generic does the trick. Do you mind prefixing all the .exp's with cilkplus_ or something similar? [Perhaps someone can pontificate as to what the actual problem is here and describe it. Dejagnu is a mystery to me.] I doubt it's anything really related to dejagnu and is more related to how the make check target is constructed in gcc/Makefile.in. It's brutally ugly. I recently had to swap in some dejagnu/tcl basics; and I did everything possible to forget it as soon as possible. I'd really forgotten how much I hated writing tcl code. Jeff
