Re: C++ interop issue due to non-null pointers
On 06/17/2014 05:00 PM, Jakub Jelinek wrote: GCC will likely not optimize it away at this point, but having code with undefined behavior is just asking for future trouble. Just use "" instead? It's always const and may lack sufficient alignment. The former isn't a problem in C++ (I think), but the alignment is an issue. Alignment for what? You don't specify any alignment to the C qsort, You're returning a T *, not a void *, and C++ requires that pointers are properly aligned even if they aren't dereferenced. -- Florian Weimer / Red Hat Product Security Team
Re: C++ interop issue due to non-null pointers
On Wed, Jun 18, 2014 at 09:18:10AM +0200, Florian Weimer wrote: > On 06/17/2014 05:00 PM, Jakub Jelinek wrote: > > >>>GCC will likely not optimize it away at this point, but having code with > >>>undefined behavior is just asking for future trouble. Just use "" instead? > >> > >>It's always const and may lack sufficient alignment. The former isn't a > >>problem in C++ (I think), but the alignment is an issue. > > > >Alignment for what? You don't specify any alignment to the C qsort, > > You're returning a T *, not a void *, and C++ requires that pointers are > properly aligned even if they aren't dereferenced. C qsort doesn't return anything and the comparison function returns int. extern void qsort (void *__base, size_t __nmemb, size_t __size, __compar_fn_t __compar) __nonnull ((1, 4)); It is the C qsort that has undefined behavior if the 1st or 4th argument is NULL. So I don't see what is wrong on: qsort (ptr ? (void *) ptr : "", nmemb, sizeof (*ptr), compar); Jakub
Re: C++ interop issue due to non-null pointers
On 06/18/2014 09:24 AM, Jakub Jelinek wrote: You're returning a T *, not a void *, and C++ requires that pointers are properly aligned even if they aren't dereferenced. C qsort doesn't return anything and the comparison function returns int. extern void qsort (void *__base, size_t __nmemb, size_t __size, __compar_fn_t __compar) __nonnull ((1, 4)); It is the C qsort that has undefined behavior if the 1st or 4th argument is NULL. So I don't see what is wrong on: qsort (ptr ? (void *) ptr : "", nmemb, sizeof (*ptr), compar); Ah, we have a misunderstanding. I assumed you were talking about the data() member of std::vector. -- Florian Weimer / Red Hat Product Security Team
Re: [GSoC] decision tree first steps
On Tue, Jun 17, 2014 at 3:15 PM, Richard Biener wrote: > > On Tue, Jun 17, 2014 at 12:21 AM, Prathamesh Kulkarni > wrote: > > On Mon, Jun 16, 2014 at 4:45 PM, Richard Biener > > wrote: > >> > >> > * Patterns requiring GENERIC support like cond_expr > >> > I am not sure about how to handle these patterns. I was thinking about > >> > handling them after we have > >> > GENERIC code generation in place. > >> > >> Yeah, though handling GENERIC for matching is as simple as > >> emitting the code check twice, the 2nd check off the an else > >> from the if (TREE_CODE (op) == SSA_NAME). That is, > >> arrange for expr::gen_gimple_match_dt to emit > >> > >>if (TREE_CODE (...) == SSA_NAME) > >> { > >> gimple def_stmt = SSA_NAME_DEF_STMT (...); > >> if (is_gimple_assign (def_stmt) && gimple_assign_rhs_code > >> (def_stmt) == ) > >> { > >> > >> } > >>else (TREE_CODE (...) == ) > >> { > >> > >> } > >> > >> which would require some refactoring in the generator. As for refactoring > >> it I'd not hook the gen_gimple_match_dt off the AST operands but > >> inline it in the decision tree traversal routine - that also makes the > >> commoning above easier. > > Thanks, I shall get started on this. > > Good. You also miss the special-casing of REALPART_EXPR, > IMAGPART_EXPR, VIEW_CONVERT_EXPR and BIT_FIELD_REF > operand extraction as you say below. > > >> > >> Btw, I checked what we generate for (bogus) > >> > >> (match_and_simplify > >> (MINUS_EXPR (PLUS_EXPR@2 @0 @1) @2) > >> @1) > >> > >> and the DT looks like > >> > >> root, 1 > >> |--operand: MINUS_EXPR, 1 > >> |operand: true, 1 > >> |--operand: PLUS_EXPR, 1 > >> |operand: true, 1 > >> |--operand: true, 1 > >> |operand: match(1), 1 > >> |--simplify_0, 0 > >> > >> though I expected > >> > >> root, 1 > >> |--operand: MINUS_EXPR, 1 > >> |operand: PLUS_EXPR, 1 > >> |--operand: true, 1 > >> |operand: true, 1 > >> |--operand: match(1), 1 > >> |simplify_0, 0 > >> > >> that is, I wonder where the extra "true" comes from. > > Thanks, fixed it in the current patch. > > Thanks. > > >> > >> > >> For > >> > >> (match_and_simplify > >> (MINUS_EXPR @2 (PLUS_EXPR@2 @0 @1)) > >> @1) > >> > >> I get > >> > >> root, 1 > >> |--operand: MINUS_EXPR, 1 > >> |operand: true, 1 > >> |--operand: match(1), 1 > >> |operand: PLUS_EXPR, 1 > >> |--operand: true, 1 > >> |operand: true, 1 > >> |--simplify_0, 0 > >> > >> which looks good to me. > >> > >> There is still a fallthru for all match failures but the DT should ensure > >> that once any of the checks is false we can terminate - that is, > >> we never have to backtrack. Sth simple like > >> > >> --- genmatch.c.orig 2014-06-16 12:57:38.401890454 +0200 > >> +++ genmatch.c 2014-06-16 12:58:03.451888730 +0200 > >> @@ -816,6 +816,7 @@ > >>unsigned i; > >>for (i = 0; i < kids.length (); ++i) > >> kids[i]->gen_gimple (f); > >> + fprintf (f, "return false;\n"); > >> > >> > >>for (i = 0; i < n_braces; ++i) > >> fprintf (f, "}\n"); > >> > >> So overall I think we are ok sofar and don't need major changes in > >> the algorithm. > >> > >> I'd say add the missing call support and we're good to go ripping out > >> the non-decision tree path. > >> > >> I'm happy to do some of the refactoring that I'd like to see myself > >> so you can concentrate on pattern implementing for phase 2. But > >> feel free to do some of it yourself. > >> > >> > Small point: I was wondering if it's a good idea to slightly change > >> > the syntax of pattern to sth like: > >> > match-expression -> transform ? > >> > eg: > >> > (minus (plus @0 @1) @1) -> @0 > >> > Looks smaller -:) > >> > >> Well, I suppose we stay with what we have here. > > The attached patch, adds support for built-in functions, and fixes > > insertion bug in decision tree. > > The insertion in decision tree is carried out during preorder traversal > > of AST (insert_operand), so it avoids generating preorder traversal in > > vector (removed walk_operand_preorder and > > lower_capture). For now have put (parent, pos, preorder_level) in > > separate struct operand_info, and added instance > > of this struct to operand (struct operand_info opinfo). operand_info > > is computed during preorder traversal > > (insert_operand), so parsing routines are not concerned with it. > > Eventually we should probably move > > matching code on decision tree nodes. For convenience of tracking > > patterns, I have numbered them in match.pd. > > Ok. > > > * Testing > > Total patterns in match.pd - 58 > > Total test cases: 4 (match-1.c), 32 (match-decision-tree.c), match-2.c > > is screwed. > > How is it screwed? I see it all pass ... The regexes are not written correctly. Multiple patterns have same regex in scan-tree-dump, so even if a particular test-case fails in isolation, it shows PASS when pl
Re: [GSoC] decision tree first steps
On Wed, Jun 18, 2014 at 11:21 AM, Prathamesh Kulkarni wrote: > On Tue, Jun 17, 2014 at 3:15 PM, Richard Biener > wrote: >> >> On Tue, Jun 17, 2014 at 12:21 AM, Prathamesh Kulkarni >> wrote: >> > On Mon, Jun 16, 2014 at 4:45 PM, Richard Biener >> > wrote: >> >> >> >> > * Patterns requiring GENERIC support like cond_expr >> >> > I am not sure about how to handle these patterns. I was thinking about >> >> > handling them after we have >> >> > GENERIC code generation in place. >> >> >> >> Yeah, though handling GENERIC for matching is as simple as >> >> emitting the code check twice, the 2nd check off the an else >> >> from the if (TREE_CODE (op) == SSA_NAME). That is, >> >> arrange for expr::gen_gimple_match_dt to emit >> >> >> >>if (TREE_CODE (...) == SSA_NAME) >> >> { >> >> gimple def_stmt = SSA_NAME_DEF_STMT (...); >> >> if (is_gimple_assign (def_stmt) && gimple_assign_rhs_code >> >> (def_stmt) == ) >> >> { >> >> >> >> } >> >>else (TREE_CODE (...) == ) >> >> { >> >> >> >> } >> >> >> >> which would require some refactoring in the generator. As for refactoring >> >> it I'd not hook the gen_gimple_match_dt off the AST operands but >> >> inline it in the decision tree traversal routine - that also makes the >> >> commoning above easier. >> > Thanks, I shall get started on this. >> >> Good. You also miss the special-casing of REALPART_EXPR, >> IMAGPART_EXPR, VIEW_CONVERT_EXPR and BIT_FIELD_REF >> operand extraction as you say below. >> >> >> >> >> Btw, I checked what we generate for (bogus) >> >> >> >> (match_and_simplify >> >> (MINUS_EXPR (PLUS_EXPR@2 @0 @1) @2) >> >> @1) >> >> >> >> and the DT looks like >> >> >> >> root, 1 >> >> |--operand: MINUS_EXPR, 1 >> >> |operand: true, 1 >> >> |--operand: PLUS_EXPR, 1 >> >> |operand: true, 1 >> >> |--operand: true, 1 >> >> |operand: match(1), 1 >> >> |--simplify_0, 0 >> >> >> >> though I expected >> >> >> >> root, 1 >> >> |--operand: MINUS_EXPR, 1 >> >> |operand: PLUS_EXPR, 1 >> >> |--operand: true, 1 >> >> |operand: true, 1 >> >> |--operand: match(1), 1 >> >> |simplify_0, 0 >> >> >> >> that is, I wonder where the extra "true" comes from. >> > Thanks, fixed it in the current patch. >> >> Thanks. >> >> >> >> >> >> >> For >> >> >> >> (match_and_simplify >> >> (MINUS_EXPR @2 (PLUS_EXPR@2 @0 @1)) >> >> @1) >> >> >> >> I get >> >> >> >> root, 1 >> >> |--operand: MINUS_EXPR, 1 >> >> |operand: true, 1 >> >> |--operand: match(1), 1 >> >> |operand: PLUS_EXPR, 1 >> >> |--operand: true, 1 >> >> |operand: true, 1 >> >> |--simplify_0, 0 >> >> >> >> which looks good to me. >> >> >> >> There is still a fallthru for all match failures but the DT should ensure >> >> that once any of the checks is false we can terminate - that is, >> >> we never have to backtrack. Sth simple like >> >> >> >> --- genmatch.c.orig 2014-06-16 12:57:38.401890454 +0200 >> >> +++ genmatch.c 2014-06-16 12:58:03.451888730 +0200 >> >> @@ -816,6 +816,7 @@ >> >>unsigned i; >> >>for (i = 0; i < kids.length (); ++i) >> >> kids[i]->gen_gimple (f); >> >> + fprintf (f, "return false;\n"); >> >> >> >> >> >>for (i = 0; i < n_braces; ++i) >> >> fprintf (f, "}\n"); >> >> >> >> So overall I think we are ok sofar and don't need major changes in >> >> the algorithm. >> >> >> >> I'd say add the missing call support and we're good to go ripping out >> >> the non-decision tree path. >> >> >> >> I'm happy to do some of the refactoring that I'd like to see myself >> >> so you can concentrate on pattern implementing for phase 2. But >> >> feel free to do some of it yourself. >> >> >> >> > Small point: I was wondering if it's a good idea to slightly change >> >> > the syntax of pattern to sth like: >> >> > match-expression -> transform ? >> >> > eg: >> >> > (minus (plus @0 @1) @1) -> @0 >> >> > Looks smaller -:) >> >> >> >> Well, I suppose we stay with what we have here. >> > The attached patch, adds support for built-in functions, and fixes >> > insertion bug in decision tree. >> > The insertion in decision tree is carried out during preorder traversal >> > of AST (insert_operand), so it avoids generating preorder traversal in >> > vector (removed walk_operand_preorder and >> > lower_capture). For now have put (parent, pos, preorder_level) in >> > separate struct operand_info, and added instance >> > of this struct to operand (struct operand_info opinfo). operand_info >> > is computed during preorder traversal >> > (insert_operand), so parsing routines are not concerned with it. >> > Eventually we should probably move >> > matching code on decision tree nodes. For convenience of tracking >> > patterns, I have numbered them in match.pd. >> >> Ok. >> >> > * Testing >> > Total patterns in match.pd - 58 >> > Total test cases: 4 (match-1.c), 32 (match-decision-tree.c), match-2.c >> > is screwed.
Re: regs_used estimation in IVOPTS seriously flawed
On Tue, Jun 17, 2014 at 4:59 PM, Bingfeng Mei wrote: > Hi, > I am looking at a performance regression in our code. A big loop produces > and uses a lot of temporary variables inside the loop body. The problem > appears that IVOPTS pass creates even more induction variables (from original > 2 to 27). It causes a lot of register spilling later and performance > take a severe hit. I looked into tree-ssa-loop-ivopts.c, it does call > estimate_reg_pressure_cost function to take # of registers into > consideration. The second parameter passed as data->regs_used is supposed > to represent old register usage before IVOPTS. > > return size + estimate_reg_pressure_cost (size, data->regs_used, > data->speed, > data->body_includes_call); > > In this case, it is mere 2 by following calculation. Essentially, it only > counts > all loop invariant registers, ignoring all registers produced/used inside the > loop. > > n = 0; > for (psi = gsi_start_phis (loop->header); !gsi_end_p (psi); gsi_next (&psi)) > { > phi = gsi_stmt (psi); > op = PHI_RESULT (phi); > > if (virtual_operand_p (op)) > continue; > > if (get_iv (data, op)) > continue; > > n++; > } > > EXECUTE_IF_SET_IN_BITMAP (data->relevant, 0, j, bi) > { > struct version_info *info = ver_info (data, j); > > if (info->inv_id && info->has_nonlin_use) > n++; > } > > data->regs_used = n; > > I believe how regs_used is calculated is seriously flawed, > or estimate_reg_pressure_cost is problematic if n_old is > only supposed to be loop invariant registers. Either way, > it affects how IVOPTS makes decision and could result in > worse code. What do you think? Any idea on how to improve > this? Well, it's certainly a lower bound on the number of registers live through the whole loop execution (thus over the backedge). So they have the same cost as an induction variable as far as register pressure is concerned. What it doesn't account for is the maximum number of live registers anywhere in the loop body - but that is hard to estimate at this point in the compilation. You could compute the maximum number of live SSA names which could be an upper bound on the register pressure - but that needs liveness analysis which is expensive also that upper bound is probably way too high. So I think the current logic is sensible and simple. It's just not perfect. Maybe it's just the cost function of the IV set choosen that needs to be adjusted to account for the number of IVs in a non-linear way? That is, adjust ivopts_global_cost_for_size which just adds size to sth that pessimizes more IVs even more like size * (1 + size / (1 + data->regs_used)) or simply size ** (1. + eps) with a suitable eps < 2. Richard. > > Thanks, > Bingfeng >
RE: regs_used estimation in IVOPTS seriously flawed
> -Original Message- > From: Richard Biener [mailto:richard.guent...@gmail.com] > Sent: 18 June 2014 12:36 > To: Bingfeng Mei > Cc: gcc@gcc.gnu.org > Subject: Re: regs_used estimation in IVOPTS seriously flawed > > On Tue, Jun 17, 2014 at 4:59 PM, Bingfeng Mei wrote: > > Hi, > > I am looking at a performance regression in our code. A big loop > produces > > and uses a lot of temporary variables inside the loop body. The > problem > > appears that IVOPTS pass creates even more induction variables (from > original > > 2 to 27). It causes a lot of register spilling later and performance > > take a severe hit. I looked into tree-ssa-loop-ivopts.c, it does call > > estimate_reg_pressure_cost function to take # of registers into > > consideration. The second parameter passed as data->regs_used is > supposed > > to represent old register usage before IVOPTS. > > > > return size + estimate_reg_pressure_cost (size, data->regs_used, > data->speed, > > data->body_includes_call); > > > > In this case, it is mere 2 by following calculation. Essentially, it > only counts > > all loop invariant registers, ignoring all registers produced/used > inside the loop. > > > > n = 0; > > for (psi = gsi_start_phis (loop->header); !gsi_end_p (psi); gsi_next > (&psi)) > > { > > phi = gsi_stmt (psi); > > op = PHI_RESULT (phi); > > > > if (virtual_operand_p (op)) > > continue; > > > > if (get_iv (data, op)) > > continue; > > > > n++; > > } > > > > EXECUTE_IF_SET_IN_BITMAP (data->relevant, 0, j, bi) > > { > > struct version_info *info = ver_info (data, j); > > > > if (info->inv_id && info->has_nonlin_use) > > n++; > > } > > > > data->regs_used = n; > > > > I believe how regs_used is calculated is seriously flawed, > > or estimate_reg_pressure_cost is problematic if n_old is > > only supposed to be loop invariant registers. Either way, > > it affects how IVOPTS makes decision and could result in > > worse code. What do you think? Any idea on how to improve > > this? > > Well, it's certainly a lower bound on the number of registers > live through the whole loop execution (thus over the backedge). > So they have the same cost as an induction variable as far > as register pressure is concerned. > > What it doesn't account for is the maximum number of live > registers anywhere in the loop body - but that is hard to > estimate at this point in the compilation. You could compute > the maximum number of live SSA names which could be > an upper bound on the register pressure - but that needs > liveness analysis which is expensive also that upper bound > is probably way too high. > Yes, I agree it is hard and probably expensive at this stage of compilation to do accurate analysis. But it could be quite useful for many tree-level loop optimizations, even just a half-accurate estimation for register pressure, as also discussed in another thread a few days ago. > So I think the current logic is sensible and simple. It's just > not perfect. > > Maybe it's just the cost function of the IV set choosen that > needs to be adjusted to account for the number of IVs > in a non-linear way? That is, adjust ivopts_global_cost_for_size > which just adds size to sth that pessimizes more IVs even > more like size * (1 + size / (1 + data->regs_used)) or > simply size ** (1. + eps) with a suitable eps < 2. > I am going to try a few cost functions as you suggested. Maybe also just count all SSA together and divide it by a factor. Thanks, Bingfeng
Re: [GSoC] How to get started with the isl code generation
Hi Tobias, I made a separate patch and rebased the previous one. They are attached to this letter. > I am surprised. Are all these includes really needed to get _this_ patch > compile? (I asked this before). I saw your previous comment related to this and the following includes were removed: isl/list.h, isl/constraint.h, isl/ilp.h, isl/aff.h, diagnostic-core.h, tree-ssa-loop-manip.h, tree-into-ssa.h, tree-chrec.h, tree-scalar-evolution.h. However, it seems that if we want to use the struct scop_p from graphite-poly.h, we have to include sese.h, which requires tree-data-ref.h, cfgloop.h, tree-ssa-loop.h, gimple-iterator.h, gimple.h, is-a.h, gimple-expr.h, internal-fn.h, tree-ssa-alias.h, basic-block.h, tree.h, coretypes.h, system.h, config.h We also have to include tree-pass.h, if we want to use timevar_push, timevar_pop to to measure elapsed time (it was used previously by graphite-clast-to-gimple.c). > This is ugly, but there is little we can do about it. Maybe you can ask > on the mailing list if there is a way to write this in multiple lines? The patch was reduced, but I didn't found out how to make the regexp easier to read. I wrote about this to the community. -- Cheers, Roman Gareev ChangeLog_entry1 Description: Binary data ChangeLog_entry2 Description: Binary data patch1 Description: Binary data patch2 Description: Binary data
[GSoC] question about regular expressions
Dear gcc contributors, could you please advise how to better write the following testcase? After the compilation with -O2 -fdump-tree-graphite-all -fgraphite-identity -fgraphite-code-generator=isl the dump file should contain the following text ISL AST generated by ISL: for (int c1 = 0; c1 < n - 1; c1 += 1) for (int c3 = 0; c3 < n; c3 += 1) S_5(c1, c3);" 1 "graphite"} } I am using the following code to check it: diff --git a/gcc/testsuite/gcc.dg/graphite/isl-codegen-loop-dumping.c b/gcc/testsuite/gcc.dg/graphite/isl-codegen-loop-dumping.c new file mode 100644 index 000..1bb0349 --- /dev/null +++ b/gcc/testsuite/gcc.dg/graphite/isl-codegen-loop-dumping.c @@ -0,0 +1,16 @@ +/* { dg-options "-O2 -fdump-tree-graphite-all -fgraphite-identity -fgraphite-code-generator=isl" } */ + +int +main (int n, int *a) +{ + int i, j; + + for (i = 0; i < n - 1; i++) +for (j = 0; j < n; j++) + a[j] = i + n; + + return 0; +} + +/* { dg-final { scan-tree-dump-times "ISL AST generated by ISL: \nfor \\(int c1 = 0; c1 < n - 1; c1 \\+= 1\\)\n for \\(int c3 = 0; c3 < n; c3 \\+= 1\\)\nS_5\\(c1, c3\\);" 1 "graphite"} } */ +/* { dg-final { cleanup-tree-dump "graphite" } } */ However, I want to make the regular expression easier to read and similar to the following multiline form: +/* { dg-final { scan-tree-dump-times "ISL AST generated by ISL: \n ? for \\(int c1 = 0; c1 < n - 1; c1 \\+= 1\\)\n ? for \\(int c3 = 0; c3 < n; c3 \\+= 1\\)\n ?S_5\\(c1, c3\\);" 1 "graphite"} ? } */ Is it possible? Where can I find the description of regular expressions used in scan-tree-dump-times? I would be very grateful for your comments. -- Cheers, Roman Gareev
Re: [GSoC] How to get started with the isl code generation
On 18/06/2014 15:22, Roman Gareev wrote: Hi Tobias, I made a separate patch and rebased the previous one. They are attached to this letter. I am surprised. Are all these includes really needed to get _this_ patch compile? (I asked this before). I saw your previous comment related to this and the following includes were removed: isl/list.h, isl/constraint.h, isl/ilp.h, isl/aff.h, diagnostic-core.h, tree-ssa-loop-manip.h, tree-into-ssa.h, tree-chrec.h, tree-scalar-evolution.h. However, it seems that if we want to use the struct scop_p from graphite-poly.h, we have to include sese.h, which requires tree-data-ref.h, cfgloop.h, tree-ssa-loop.h, gimple-iterator.h, gimple.h, is-a.h, gimple-expr.h, internal-fn.h, tree-ssa-alias.h, basic-block.h, tree.h, coretypes.h, system.h, config.h We also have to include tree-pass.h, if we want to use timevar_push, timevar_pop to to measure elapsed time (it was used previously by graphite-clast-to-gimple.c). This is ugly, but there is little we can do about it. Maybe you can ask on the mailing list if there is a way to write this in multiple lines? The patch was reduced, but I didn't found out how to make the regexp easier to read. I wrote about this to the community. Nice. These patches look good to me. One last question. How did you test them? Specifically, do they compile with the oldest isl version supported by gcc trunk? Or do we need to update isl as well? Cheers, Tobias
gfortran option -fno-automatic and -finit-local-zero
Hello, First of all thanks so much for all your hard work! I'm compiling a bunch of legacy fortran that relies on SAVE semantics from relic compilers. I'm using the -fno-automatic option to replicate that behavior. I also was using the -finit-local-zero option, assuming that it would initialize local variables upon first use, and then save their values in subsequent calls due to the -fno-automatic option. However, I found that -finit-local-zero would initialize local variables to zero upon each subroutine call regardless of whether -fno-automatic was used or not. I don't know if this is expected behavior, but I thought you may want to mention it in the gfortran man page. Thanks again! -Jeff Chase
Re: [GSoC] How to get started with the isl code generation
I used trunk and compiled these patches only with isl 0.12 and ClooG 0.18.1. Which versions of these libraries are need to be checked for compatibility? -- Cheers, Roman Gareev
Re: [GSoC] How to get started with the isl code generation
On Jun 18, 2014 6:03 PM, Roman Gareev wrote: > > I used trunk and compiled these patches only with isl 0.12 and ClooG > 0.18.1. Which versions of these libraries are need to be checked for > compatibility? That's fine. Please post the patches for wider review at gcc patches. Also mention that your copyright assignment has been processed. Thanks Tobias > > -- > Cheers, Roman Gareev
ICE with atomic_store
Hello All, The following code: #include struct s1_st { char* i_name; struct s1_st* i_foo; }; void clear_s1 (struct s1_st*s) { __atomic_store(s->i_name, NULL, __ATOMIC_SEQ_CST); } gives an ICE when compiled (on Debian/Sid/amd64) by GCC 4.8 & 4.9 (I did not test it on the trunk yet) (For some strange reason, probably on my side, I'm not able to change the password on bugzilla. So I'm reporting that bug here, sorry for the inconvenience). Cheers. -- Basile STARYNKEVITCH http://starynkevitch.net/Basile/ email: basilestarynkevitchnet mobile: +33 6 8501 2359 8, rue de la Faiencerie, 92340 Bourg La Reine, France *** opinions {are only mines, sont seulement les miennes} ***
Re: ICE with atomic_store
On 06/18/2014 12:59 PM, Basile Starynkevitch wrote: Hello All, The following code: #include struct s1_st { char* i_name; struct s1_st* i_foo; }; void clear_s1 (struct s1_st*s) { __atomic_store(s->i_name, NULL, __ATOMIC_SEQ_CST); } gives an ICE when compiled (on Debian/Sid/amd64) by GCC 4.8 & 4.9 (I did not test it on the trunk yet) (For some strange reason, probably on my side, I'm not able to change the password on bugzilla. So I'm reporting that bug here, sorry for the inconvenience). Cheers. First, thats an error... the second parameter to __atomic_store must be a pointer to memory, not NULL. We store the contents of that location to s->i_name. Perhaps it was meant to be __atomic_store_n () ? That doesn't excuse the exception however. Looks like void type has a NULL TYPE_SIZE_UNIT, which causes tree_to_uhwi to segfault. could be fixed with something like this.. try it. I get a.c: In function ‘clear_s1’: a.c:9:6: error: size mismatch in argument 2 of ‘__atomic_store’ __atomic_store(s->i_name, NULL, __ATOMIC_SEQ_CST); Thoroughly untested of course :-) Andrew * c-common,c (get_atomic_generic_size): Handle pointer to 0 size. Index: c-family/c-common.c === *** c-family/c-common.c (revision 211758) --- c-family/c-common.c (working copy) *** get_atomic_generic_size (location_t loc, *** 10462,10467 --- 10462,10468 { int size; tree type = TREE_TYPE ((*params)[x]); + tree type_size; /* __atomic_compare_exchange has a bool in the 4th position, skip it. */ if (n_param == 6 && x == 3) continue; *** get_atomic_generic_size (location_t loc, *** 10471,10477 function); return 0; } ! size = tree_to_uhwi (TYPE_SIZE_UNIT (TREE_TYPE (type))); if (size != size_0) { error_at (loc, "size mismatch in argument %d of %qE", x + 1, --- 10472,10480 function); return 0; } ! ! type_size = TYPE_SIZE_UNIT (TREE_TYPE (type)); ! size = type_size ? tree_to_uhwi (type_size) : 0; if (size != size_0) { error_at (loc, "size mismatch in argument %d of %qE", x + 1,
gcc-4.9-20140618 is now available
Snapshot gcc-4.9-20140618 is now available on ftp://gcc.gnu.org/pub/gcc/snapshots/4.9-20140618/ and on various mirrors, see http://gcc.gnu.org/mirrors.html for details. This snapshot has been generated from the GCC 4.9 SVN branch with the following options: svn://gcc.gnu.org/svn/gcc/branches/gcc-4_9-branch revision 211810 You'll find: gcc-4.9-20140618.tar.bz2 Complete GCC MD5=2a3c50ca43902d4a11d788a21e163801 SHA1=c247436ec884408999fc2952bbe13d243d83d939 Diffs from 4.9-20140611 are available in the diffs/ subdirectory. When a particular snapshot is ready for public consumption the LATEST-4.9 link is updated and a message is sent to the gcc list. Please do not use a snapshot before it has been announced that way.