* [PATCH] omp-low.c split @ 2016-12-09 13:08 Martin Jambor 2016-12-09 13:25 ` Alexander Monakov ` (2 more replies) 0 siblings, 3 replies; 18+ messages in thread From: Martin Jambor @ 2016-12-09 13:08 UTC (permalink / raw) To: GCC Patches Cc: Jakub Jelinek, Cesar Philippidis, Alexander Monakov, Thomas Schwinge [-- Attachment #1: Type: text/plain, Size: 15336 bytes --] Hello, this is the promised attempt at splitting omp-low.c that turns: gcc$ wc -l omp-*.[ch] 21129 omp-low.c 44 omp-low.h 1726 omp-simd-clone.c into: gcc$ wc -l omp-*.[ch] 1718 omp-device.c 30 omp-device.h 8195 omp-expand.c 32 omp-expand.h 650 omp-general.c 91 omp-general.h 1407 omp-grid.c 27 omp-grid.h 9240 omp-low.c 31 omp-low.h 1726 omp-simd-clone.c Basically, I tried to: - move everything that seemed to part of expand or somehow expand-related (usually because it uses omp_region data structure) to a new file omp-expand.c, - move everything that is part of pass_oacc_device_lower, pass_omp_device_lower and pass_omp_target_link to a new file omp-device.h, - move all pre-lowering gridification stuff to a new file omp-grid.c. I have moved expansion of HSA giridified kernels to omp-expand.c because moving them elsewhere would mean exposing omp_region somehow. We can always do that as a follow-up. - I moved stuff that was used from all over the place to a new file omp-general.c (unless it would mean exposing omp_region or omp_context types). - I have added a header file associated with each new .c file for symbols exported from them. I have renamed all exported functions so that all of them start with either omp_ or oacc_ - I have removed includes in omp-low.c that no were no longer necessary. - I have removed static inline function get_base_type that was apparently no longer used. - The new files should not have any trailing whitespace and I corrected a (very) few instances of bad horizontal whitespace but I tried to keep the actual changes to a minimum. I am opened to suggestions what to do differently, names of the file are for example of course subject to discussion, and I absolutely welcome any review and checking, for one I am not going to pretend I understand the stuff I put into omp-device.c. If however there is consensus that we should do something like this, I would like to ask the community to freeze omp-low.c file until this gets committed, I hope you understand that I am afraid of any conflicts. I have bootstrapped and tested a very similar patch (before #include pruning) on x86_64-linux and on aarch64-linux, re-runs of those are currently underway. Please find the changelog below and the compressed patch (1.1 MB before gzipping) attached. After all feedback is resolved, I would like to commit this to trunk. Thanks, Martin 2016-12-09 Martin Jambor <mjambor@suse.cz> * omp-general.h: New file. * omp-general.c: New file. * omp-expand.h: Likewise. * omp-expand.c: Likewise. * omp-device.h: Likewise. * omp-device.c: Likewise. * omp-grid.c: Likewise. * omp-grid.c: Likewise. * omp-low.h: Include omp-general.h and omp-grid.h. Removed includes of params.h, symbol-summary.h, lto-section-names.h, cilk.h, tree-eh.h, ipa-prop.h, tree-cfgcleanup.h, cfgloop.h, except.h, expr.h, stmt.h, varasm.h, calls.h, explow.h, dojump.h, flags.h, tree-into-ssa.h, tree-cfg.h, cfganal.h, alias.h, emit-rtl.h, optabs.h, expmed.h, alloc-pool.h, cfghooks.h, rtl.h and memmodel.h. (omp_find_combined_for): Declare. (find_omp_clause): Renamed to omp_find_clause and moved to omp-general.h. (free_omp_regions): Renamed to omp_free_regions and moved to omp-expand.h. (replace_oacc_fn_attrib): Renamed to oacc_replace_fn_attrib and moved to omp-general.h. (set_oacc_fn_attrib): Renamed to oacc_set_fn_attrib and moved to omp-general.h. (build_oacc_routine_dims): Renamed to oacc_build_routine_dims and moved to omp-general.h. (get_oacc_fn_attrib): Renamed to oacc_get_fn_attrib and moved to omp-general.h. (oacc_fn_attrib_kernels_p): Moved to omp-general.h. (get_oacc_fn_dim_size): Renamed to oacc_get_fn_dim_size and moved to omp-general.c. (omp_expand_local): Moved to omp-expand.h. (make_gimple_omp_edges): Renamed to omp_make_gimple_edges and moved to omp-expand.h. (omp_finish_file): Moved to omp-device.h. (default_goacc_validate_dims): Renamed to oacc_default_goacc_validate_dims and moved to omp-device.h. (offload_funcs, offload_vars): Moved to omp-device.h. * omp-low.c: Include omp-general.h, omp-device.h and omp-grid.h. (omp_region): Moved to omp-expand.c. (omp_for_data_loop): Moved to omp-general.h. (omp_for_data): Likewise. (oacc_loop): Moved to omp-device.c. (oacc_loop_flags): Moved to omp-general.h. (offload_funcs, offload_vars): Moved to omp-device.c. (root_omp_region): Moved to omp-expand.c. (omp_any_child_fn_dumped): Likewise. (find_omp_clause): Renamed to omp_find_clause and moved to omp-general.c. (is_combined_parallel): kMoved to omp-expand.c. (is_reference): Renamed to omp_is_reference and and moved to omp-general.c. (adjust_for_condition): Renamed to omp_adjust_for_condition and moved to omp-general.c. (get_omp_for_step_from_incr): Renamed to omp_get_for_step_from_incr and moved to omp-general.c. (extract_omp_for_data): Renamed to omp_extract_for_data and moved to omp-general.c. (workshare_safe_to_combine_p): Moved to omp-expand.c. (omp_adjust_chunk_size): Likewise. (get_ws_args_for): Likewise. (get_base_type): Removed. (dump_omp_region): Moved to omp-expand.c. (debug_omp_region): Likewise. (debug_all_omp_regions): Likewise. (new_omp_region): Likewise. (free_omp_region_1): Likewise. (free_omp_regions): Renamed to omp_free_regions and moved to omp-expand.c. (find_combined_for): Renamed to omp_find_combined_for, made global. (build_omp_barrier): Renamed to omp_build_barrier and moved to omp-general.c. (omp_max_vf): Moved to omp-general.c. (omp_max_simt_vf): Likewise. (gimple_build_cond_empty): Moved to omp-expand.c. (parallel_needs_hsa_kernel_p): Likewise. (expand_omp_build_assign): Moved declaration to omp-expand.c. (expand_parallel_call): Moved to omp-expand.c. (expand_cilk_for_call): Likewise. (expand_task_call): Likewise. (vec2chain): Likewise. (remove_exit_barrier): Likewise. (remove_exit_barriers): Likewise. (optimize_omp_library_calls): Likewise. (expand_omp_regimplify_p): Likewise. (expand_omp_build_assign): Likewise. (expand_omp_taskreg): Likewise. (oacc_collapse): Likewise. (expand_oacc_collapse_init): Likewise. (expand_oacc_collapse_vars): Likewise. (expand_omp_for_init_counts): Likewise. (expand_omp_for_init_vars): Likewise. (extract_omp_for_update_vars): Likewise. (expand_omp_ordered_source): Likewise. (expand_omp_ordered_sink): Likewise. (expand_omp_ordered_source_sink): Likewise. (expand_omp_for_ordered_loops): Likewise. (expand_omp_for_generic): Likewise. (expand_omp_for_static_nochunk): Likewise. (find_phi_with_arg_on_edge): Likewise. (expand_omp_for_static_chunk): Likewise. (expand_cilk_for): Likewise. (expand_omp_simd): Likewise. (expand_omp_taskloop_for_outer): Likewise. (expand_omp_taskloop_for_inner): Likewise. (expand_oacc_for): Likewise. (expand_omp_for): Likewise. (expand_omp_sections): Likewise. (expand_omp_single): Likewise. (expand_omp_synch): Likewise. (expand_omp_atomic_load): Likewise. (expand_omp_atomic_store): Likewise. (expand_omp_atomic_fetch_op): Likewise. (expand_omp_atomic_pipeline): Likewise. (expand_omp_atomic_mutex): Likewise. (expand_omp_atomic): Likewise. (oacc_launch_pack): and moved to omp-general.c, made public. (OACC_FN_ATTRIB): Likewise. (replace_oacc_fn_attrib): Renamed to oacc_replace_fn_attrib and moved to omp-general.c. (set_oacc_fn_attrib): Renamed to oacc_set_fn_attrib and moved to omp-general.c. (build_oacc_routine_dims): Renamed to oacc_build_routine_dims and moved to omp-general.c. (get_oacc_fn_attrib): Renamed to oacc_get_fn_attrib and moved to omp-general.c. (oacc_fn_attrib_kernels_p): Moved to omp-general.c. (oacc_fn_attrib_level): Moved to omp-device.c. (get_oacc_fn_dim_size): Renamed to oacc_get_fn_dim_size and moved to omp-general.c. (get_oacc_ifn_dim_arg): Renamed to oacc_get_ifn_dim_arg and moved to omp-general.c. (mark_loops_in_oacc_kernels_region): Moved to omp-expand.c. (grid_launch_attributes_trees): Likewise. (grid_attr_trees): Likewise. (grid_create_kernel_launch_attr_types): Likewise. (grid_insert_store_range_dim): Likewise. (grid_get_kernel_launch_attributes): Likewise. (get_target_argument_identifier_1): Likewise. (get_target_argument_identifier): Likewise. (get_target_argument_value): Likewise. (push_target_argument_according_to_value): Likewise. (get_target_arguments): Likewise. (expand_omp_target): Likewise. (grid_expand_omp_for_loop): Moved to omp-grid.c. (grid_arg_decl_map): Likewise. (grid_remap_kernel_arg_accesses): Likewise. (grid_expand_target_grid_body): Likewise. (expand_omp): Renamed to omp_expand and moved to omp-expand.c. (build_omp_regions_1): Moved to omp-expand.c. (build_omp_regions_root): Likewise. (omp_expand_local): Likewise. (build_omp_regions): Likewise. (execute_expand_omp): Likewise. (pass_data_expand_omp): Likewise. (pass_expand_omp): Likewise. (make_pass_expand_omp): Likewise. (pass_data_expand_omp_ssa): Likewise. (pass_expand_omp_ssa): Likewise. (make_pass_expand_omp_ssa): Likewise. (grid_lastprivate_predicate): Renamed to omp_grid_lastprivate_predicate and moved to omp-grid.c, made public. (grid_prop): Moved to omp-grid.c. (GRID_MISSED_MSG_PREFIX): Likewise. (grid_safe_assignment_p): Likewise. (grid_seq_only_contains_local_assignments): Likewise. (grid_find_single_omp_among_assignments_1): Likewise. (grid_find_single_omp_among_assignments): Likewise. (grid_find_ungridifiable_statement): Likewise. (grid_parallel_clauses_gridifiable): Likewise. (grid_inner_loop_gridifiable_p): Likewise. (grid_dist_follows_simple_pattern): Likewise. (grid_gfor_follows_tiling_pattern): Likewise. (grid_call_permissible_in_distribute_p): Likewise. (grid_handle_call_in_distribute): Likewise. (grid_dist_follows_tiling_pattern): Likewise. (grid_target_follows_gridifiable_pattern): Likewise. (grid_remap_prebody_decls): Likewise. (grid_var_segment): Likewise. (grid_mark_variable_segment): Likewise. (grid_copy_leading_local_assignments): Likewise. (grid_process_grid_body): Likewise. (grid_eliminate_combined_simd_part): Likewise. (grid_mark_tiling_loops): Likewise. (grid_mark_tiling_parallels_and_loops): Likewise. (grid_process_kernel_body_copy): Likewise. (grid_attempt_target_gridification): Likewise. (grid_gridify_all_targets_stmt): Likewise. (grid_gridify_all_targets): Renamed to omp_grid_gridify_all_targets and moved to omp-grid.c, made public. (make_gimple_omp_edges): Renamed to omp_make_gimple_edges and moved to omp-expand.c. (add_decls_addresses_to_decl_constructor): Moved to omp-device.c. (omp_finish_file): Likewise. (oacc_thread_numbers): Likewise. (oacc_xform_loop): Likewise. (oacc_default_dims, oacc_min_dims): Likewise. (oacc_parse_default_dims): Likewise. (oacc_validate_dims): Likewise. (new_oacc_loop_raw): Likewise. (new_oacc_loop_outer): Likewise. (new_oacc_loop): Likewise. (new_oacc_loop_routine): Likewise. (finish_oacc_loop): Likewise. (free_oacc_loop): Likewise. (dump_oacc_loop_part): Likewise. (dump_oacc_loop): Likewise. (debug_oacc_loop): Likewise. (oacc_loop_discover_walk): Likewise. (oacc_loop_sibling_nreverse): Likewise. (oacc_loop_discovery): Likewise. (oacc_loop_xform_head_tail): Likewise. (oacc_loop_xform_loop): Likewise. (oacc_loop_process): Likewise. (oacc_loop_fixed_partitions): Likewise. (oacc_loop_auto_partitions): Likewise. (oacc_loop_partition): Likewise. (default_goacc_fork_join): Likewise. (default_goacc_reduction): Likewise. (execute_oacc_device_lower): Likewise. (default_goacc_validate_dims): Likewise. (default_goacc_dim_limit): Likewise. (pass_data_oacc_device_lower): Likewise. (pass_oacc_device_lower): Likewise. (make_pass_oacc_device_lower): Likewise. (execute_omp_device_lower): Likewise. (pass_data_omp_device_lower): Likewise. (pass_omp_device_lower): Likewise. (make_pass_omp_device_lower): Likewise. (pass_data_omp_target_link): Likewise. (pass_omp_target_link): Likewise. (find_link_var_op): Likewise. (pass_omp_target_link::execute): Likewise. (make_pass_omp_target_link): Likewise. * Makefile.in (OBJS): Added omp-device.o, omp-expand.o, omp-general.o and omp-grid.o. (GTFILES): Added omp-device.h, omp-device.c and omp-expand.c, removed omp-low.h. * gimple-fold.c: Include omp-general.h instead of omp-low.h. (fold_internal_goacc_dim): Adjusted calls to get_oacc_ifn_dim_arg and get_oacc_fn_dim_size to use their new names. * gimplify.c: Include omp-low.h. (omp_notice_variable): Adjust the call to get_oacc_fn_attrib to use its new name. (gimplify_omp_task): Adjusted calls to find_omp_clause to use its new name. (gimplify_omp_for): Likewise. * lto-cgraph.c: Include omp-device.h instead of omp-low.h. * toplev.c: Include omp-device.h instead of omp-low.h. * tree-cfg.c: Include omp-general.h instead of omp-low.h. Also include omp-expand.h. (make_edges_bb): Adjusted the call to make_gimple_omp_edges to use its new name. (make_edges): Adjust the call to free_omp_regions to use its new name. * tree-parloops.c: Include omp-general.h. (create_parallel_loop): Adjusted the call to set_oacc_fn_attrib to use its new name. (parallelize_loops): Adjusted the call to get_oacc_fn_attrib to use its new name. * tree-ssa-loop.c: Include omp-general.h instead of omp-low.h. (gate_oacc_kernels): Adjusted the call to get_oacc_fn_attrib to use its new name. * tree-vrp.c: Include omp-general.h instead of omp-low.h. (extract_range_basic): Adjusted calls to get_oacc_ifn_dim_arg and get_oacc_fn_dim_size to use their new names. * varpool.c: Include omp-device.h instead of omp-low.h. * gengtype.c (open_base_files): Replace omp-low.h with omp-device.h in ifiles. gcc/c-family: * c-omp.c: Include omp-general.h instead of omp-low.h. (c_finish_oacc_wait): Adjusted call to find_omp_clause to use its new name. gcc/c/ * c-parser.c: Include omp-general.h and omp-device.h instead of omp-low.h. (c_finish_oacc_routine): Adjusted call to get_oacc_fn_attrib, build_oacc_routine_dims and replace_oacc_fn_attrib to use their new names. (c_parser_oacc_enter_exit_data): Adjusted call to find_omp_clause to use its new name. (c_parser_oacc_update): Likewise. (c_parser_omp_simd): Likewise. (c_parser_omp_target_update): Likewise. * c-typeck.c: Include omp-general.h instead of omp-low.h. (c_finish_omp_cancel): Adjusted call to find_omp_clause to use its new name. (c_finish_omp_cancellation_point): Likewise. * gimple-parser.c: Do not include omp-low.h gcc/cp/ * parser.c: Include omp-general.h and omp-device.h instead of omp-low.h. (cp_parser_omp_simd): Adjusted calls to find_omp_clause to use its new name. (cp_parser_omp_target_update): Likewise. (cp_parser_oacc_declare): Likewise. (cp_parser_oacc_enter_exit_data): Likewise. (cp_parser_oacc_update): Likewise. (cp_finalize_oacc_routine): Adjusted call to get_oacc_fn_attrib, build_oacc_routine_dims and replace_oacc_fn_attrib to use their new names. * semantics.c: Include omp-general insteda of omp-low.h. (finish_omp_for): Adjusted calls to find_omp_clause to use its new name. (finish_omp_cancel): Likewise. (finish_omp_cancellation_point): Likewise. fortran/ * trans-openmp.c: Include omp-general.h. [-- Attachment #2: 0001-Split-omp-low-into-multiple-files.patch.gz --] [-- Type: application/x-gzip, Size: 232750 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-09 13:08 [PATCH] omp-low.c split Martin Jambor @ 2016-12-09 13:25 ` Alexander Monakov 2016-12-09 13:53 ` Martin Jambor 2016-12-13 11:39 ` Thomas Schwinge 2021-08-04 12:40 ` Thomas Schwinge 2 siblings, 1 reply; 18+ messages in thread From: Alexander Monakov @ 2016-12-09 13:25 UTC (permalink / raw) To: Martin Jambor Cc: GCC Patches, Jakub Jelinek, Cesar Philippidis, Thomas Schwinge Hi Martin, Just one quick question -- do you know if config/nvptx/nvptx.c needs changes with this patch? I see it has an '#include "omp-low.h"', and it seems your patch is renaming some functions -- is the intention that no interfaces used in target-specific files are changed during the split? Thanks. Alexander ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-09 13:25 ` Alexander Monakov @ 2016-12-09 13:53 ` Martin Jambor 2016-12-09 14:22 ` Jakub Jelinek 0 siblings, 1 reply; 18+ messages in thread From: Martin Jambor @ 2016-12-09 13:53 UTC (permalink / raw) To: Alexander Monakov Cc: GCC Patches, Jakub Jelinek, Cesar Philippidis, Thomas Schwinge Hi, On Fri, Dec 09, 2016 at 04:25:10PM +0300, Alexander Monakov wrote: > Hi Martin, > > Just one quick question -- do you know if config/nvptx/nvptx.c needs changes > with this patch? I see it has an '#include "omp-low.h"', and it seems your > patch is renaming some functions -- is the intention that no interfaces used in > target-specific files are changed during the split? > Unfortunately no, that file also needs to be changed, even if very slightly. Specifically, omp-general.h also needs to be included and calls to get_oacc_fn_attrib need to be changed to call oacc_get_fn_attrib. omp-low.h has to stay included for omp_reduction_init_op and omp_reduction_init which did not change. Sorry about that, it was the only file in the back-ends and I forgot about it. I have added the following to my patch but it would be great if you verified it still compiles and works as expected for you. Thanks, Martin * config/nvptx/nvptx.c: Include omp-generic.c. (nvptx_expand_call): Adjusted the call to get_oacc_fn_attrib to use its new name. (nvptx_reorg): Likewise. (nvptx_record_offload_symbol): Likewise. diff --git a/gcc/config/nvptx/nvptx.c b/gcc/config/nvptx/nvptx.c index 405a91b..17fe551 100644 --- a/gcc/config/nvptx/nvptx.c +++ b/gcc/config/nvptx/nvptx.c @@ -55,6 +55,7 @@ #include "gimple.h" #include "stor-layout.h" #include "builtins.h" +#include "omp-general.h" #include "omp-low.h" #include "gomp-constants.h" #include "dumpfile.h" @@ -1389,7 +1390,7 @@ nvptx_expand_call (rtx retval, rtx address) if (DECL_STATIC_CHAIN (decl)) cfun->machine->has_chain = true; - tree attr = get_oacc_fn_attrib (decl); + tree attr = oacc_get_fn_attrib (decl); if (attr) { tree dims = TREE_VALUE (attr); @@ -4090,7 +4091,7 @@ nvptx_reorg (void) /* Determine launch dimensions of the function. If it is not an offloaded function (i.e. this is a regular compiler), the function has no neutering. */ - tree attr = get_oacc_fn_attrib (current_function_decl); + tree attr = oacc_get_fn_attrib (current_function_decl); if (attr) { /* If we determined this mask before RTL expansion, we could @@ -4243,7 +4244,7 @@ nvptx_record_offload_symbol (tree decl) case FUNCTION_DECL: { - tree attr = get_oacc_fn_attrib (decl); + tree attr = oacc_get_fn_attrib (decl); /* OpenMP offloading does not set this attribute. */ tree dims = attr ? TREE_VALUE (attr) : NULL_TREE; ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-09 13:53 ` Martin Jambor @ 2016-12-09 14:22 ` Jakub Jelinek 2016-12-09 16:19 ` Alexander Monakov 2016-12-13 10:16 ` Martin Jambor 0 siblings, 2 replies; 18+ messages in thread From: Jakub Jelinek @ 2016-12-09 14:22 UTC (permalink / raw) To: Alexander Monakov, GCC Patches, Cesar Philippidis, Thomas Schwinge On Fri, Dec 09, 2016 at 02:53:41PM +0100, Martin Jambor wrote: > Unfortunately no, that file also needs to be changed, even if very > slightly. Specifically, omp-general.h also needs to be included and > calls to get_oacc_fn_attrib need to be changed to call > oacc_get_fn_attrib. omp-low.h has to stay included for > omp_reduction_init_op and omp_reduction_init which did not change. > > Sorry about that, it was the only file in the back-ends and I forgot > about it. I have added the following to my patch but it would be > great if you verified it still compiles and works as expected for you. The patch including this looks mostly good to me, but can we take it also as an opportunity to clean up formatting where it went wrong over the years? contrib/check_GNU_style.sh 0001-Split-omp-low-into-multiple-files.patch reports lots of issues, as always, the script is not perfect and one needs to use reasonable judgement. There are e.g. 9 lines with over 80 chars, 19 lines with blocks of 8 spaces instead of tabs, some trailing whitespaces, "Dot, space, space, new sentence", "Dot, space, space, end of comment", some of the "There should be exactly one space between function name and parenthesis" - for omp directives in comments it should stay as is, but oacc_loop_sibling_nreverse (loop->child); if (__builtin_expect(zero, false)) goto zero_iter_bb; tgt = &OMP_CLAUSE_CHAIN(c); should be fixed. Space before dot are all false positives, etc. Trailing operator seems to also have 2 real bugs. Can you post an incremental patch fixing those issues? Thanks. Jakub ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-09 14:22 ` Jakub Jelinek @ 2016-12-09 16:19 ` Alexander Monakov 2016-12-13 10:20 ` Martin Jambor 2016-12-13 10:16 ` Martin Jambor 1 sibling, 1 reply; 18+ messages in thread From: Alexander Monakov @ 2016-12-09 16:19 UTC (permalink / raw) To: Jakub Jelinek; +Cc: GCC Patches, Cesar Philippidis, Thomas Schwinge On Fri, 9 Dec 2016, Jakub Jelinek wrote: > Can you post an incremental patch fixing those issues? A few small nits I found while reading the patch. First of all, please use 'git diff --patience' (or --histogram) when generating such patches, without it the changes in omp-low.c look uglier than necessary. The comment for 'struct oacc_loop' in new file omp-device.c is misplaced: for some reason it's above the #include block. This patch doesn't seem to apply to current trunk due to a conflict in cp/parser.c. If you could create a git branch (perhaps in your personal namespace so you can freely rebase it) with this patchset, I'd appreciate it. When trying to apply the patch, git notes a few remaining whitespace issues: $ zcat 0001-Split-omp-low-into-multiple-files.patch.gz | git apply --reject - <stdin>:2734: space before tab in indent. # BLOCK 2 (PAR_ENTRY_BB) <stdin>:5346: space before tab in indent. true, GSI_SAME_STMT); <stdin>:8129: space before tab in indent. after a stmt, not before. */ <stdin>:9060: space before tab in indent. GOMP_atomic_start (); <stdin>:9061: space before tab in indent. *addr = rhs; Hope this helps. Alexander ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-09 16:19 ` Alexander Monakov @ 2016-12-13 10:20 ` Martin Jambor 0 siblings, 0 replies; 18+ messages in thread From: Martin Jambor @ 2016-12-13 10:20 UTC (permalink / raw) To: Alexander Monakov Cc: Jakub Jelinek, GCC Patches, Cesar Philippidis, Thomas Schwinge Hi, On Fri, Dec 09, 2016 at 07:18:54PM +0300, Alexander Monakov wrote: > On Fri, 9 Dec 2016, Jakub Jelinek wrote: > > Can you post an incremental patch fixing those issues? > > A few small nits I found while reading the patch. > > First of all, please use 'git diff --patience' (or --histogram) when > generating such patches, without it the changes in omp-low.c look uglier than > necessary. OK, the new patches use --patience, the result is actually quite a bit smaller. Thanks for the hint. > > The comment for 'struct oacc_loop' in new file omp-device.c is misplaced: for > some reason it's above the #include block. Oh, thanks for noticing, fixed. > > This patch doesn't seem to apply to current trunk due to a conflict in > cp/parser.c. Hmm, I don't think so? $ svn st $ svn up Updating '.': At revision 243600. $ patch --dry-run -p1 < /tmp/misc-next/pat/0001-Split-omp-low-into-multiple-files.patch checking file gcc/Makefile.in checking file gcc/c-family/c-omp.c checking file gcc/c/c-parser.c checking file gcc/c/c-typeck.c checking file gcc/c/gimple-parser.c checking file gcc/config/nvptx/nvptx.c checking file gcc/cp/parser.c checking file gcc/cp/semantics.c checking file gcc/fortran/trans-openmp.c checking file gcc/gengtype.c checking file gcc/gimple-fold.c checking file gcc/gimplify.c checking file gcc/lto-cgraph.c checking file gcc/omp-device.c checking file gcc/omp-device.h checking file gcc/omp-expand.c checking file gcc/omp-expand.h checking file gcc/omp-general.c checking file gcc/omp-general.h checking file gcc/omp-grid.c checking file gcc/omp-grid.h checking file gcc/omp-low.c checking file gcc/omp-low.h checking file gcc/toplev.c checking file gcc/tree-cfg.c checking file gcc/tree-parloops.c checking file gcc/tree-ssa-loop.c checking file gcc/tree-vrp.c checking file gcc/varpool.c $ echo $? 0 This was using the exact same file I have compressed and sent a while ago to the mailing list. > If you could create a git branch (perhaps in your personal > namespace so you can freely rebase it) with this patchset, I'd appreciate it. > Well... I do not see this as a long-term project so I do not really see much value in this. I have used git just because that is how I conveniently send patches to machines where I bootstrap them. I hope this will get committed very early so that we avoid any big conflicts in omp-low.c. But if this drags on for a while and if our git mirror recovers meanwhile, I can. > When trying to apply the patch, git notes a few remaining whitespace issues: > > $ zcat 0001-Split-omp-low-into-multiple-files.patch.gz | git apply --reject - > > <stdin>:2734: space before tab in indent. > # BLOCK 2 (PAR_ENTRY_BB) > <stdin>:5346: space before tab in indent. > true, GSI_SAME_STMT); > <stdin>:8129: space before tab in indent. > after a stmt, not before. */ > <stdin>:9060: space before tab in indent. > GOMP_atomic_start (); > <stdin>:9061: space before tab in indent. > *addr = rhs; > I hope I have addressed this when doing what Jakub suggested, please let me know if not. Thanks for looking at the big patch! Martin ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-09 14:22 ` Jakub Jelinek 2016-12-09 16:19 ` Alexander Monakov @ 2016-12-13 10:16 ` Martin Jambor 2016-12-13 10:35 ` Jakub Jelinek 2016-12-13 12:58 ` Alexander Monakov 1 sibling, 2 replies; 18+ messages in thread From: Martin Jambor @ 2016-12-13 10:16 UTC (permalink / raw) To: Jakub Jelinek Cc: Alexander Monakov, GCC Patches, Cesar Philippidis, Thomas Schwinge [-- Attachment #1: Type: text/plain, Size: 16693 bytes --] Hi, On Fri, Dec 09, 2016 at 03:22:04PM +0100, Jakub Jelinek wrote: > On Fri, Dec 09, 2016 at 02:53:41PM +0100, Martin Jambor wrote: > > Unfortunately no, that file also needs to be changed, even if very > > slightly. Specifically, omp-general.h also needs to be included and > > calls to get_oacc_fn_attrib need to be changed to call > > oacc_get_fn_attrib. omp-low.h has to stay included for > > omp_reduction_init_op and omp_reduction_init which did not change. > > > > Sorry about that, it was the only file in the back-ends and I forgot > > about it. I have added the following to my patch but it would be > > great if you verified it still compiles and works as expected for you. > > The patch including this looks mostly good to me, but can we take it > also as an opportunity to clean up formatting where it went wrong over the > years? Sure. I have found out that I myself had erroneously my editor set to warn only after I exceeded 81 characters on a line, so a few of those violations were actually my fault. (I have actually fixed this without noticing a few weeks ago but I'm afraid I have added quite a few lines like that in the meantime.) > contrib/check_GNU_style.sh 0001-Split-omp-low-into-multiple-files.patch > reports lots of issues, as always, the script is not perfect and one needs > to use reasonable judgement. > There are e.g. 9 lines with over 80 chars, 19 lines with blocks of 8 spaces > instead of tabs, some trailing whitespaces, "Dot, space, space, new > sentence", "Dot, space, space, end of comment", some of the > "There should be exactly one space between function name and parenthesis" > - for omp directives in comments it should stay as is, but > oacc_loop_sibling_nreverse (loop->child); > if (__builtin_expect(zero, false)) goto zero_iter_bb; > tgt = &OMP_CLAUSE_CHAIN(c); > should be fixed. > Space before dot are all false positives, etc. > Trailing operator seems to also have 2 real bugs. > > Can you post an incremental patch fixing those issues? I'm attaching a refreshed patch (on top of revision 243504, git mirror commit-id 2f757f2) and another one which fixes many of the violations reported by contrib/check_GNU_style.sh. Both patches were generated from git with --patience parameter, as Alex suggested, which actually resulted in smaller files, both in terms of the actual size and line numbers. I have also moved the comment above includes in omp-device.c which Alex found to the appropriate place. When I run the combined patch through check_GNU_style.sh, I see: - "Space before dot" errors which are in comments and thus false positives. - "Sentences should end with a dot. Dot, space, space, end of the comment" errors, but all of them are short comments which do not form a sentence, and reasonably so, and thus do not end with a dot. (I have fixed a two or three which were sentences and did not end with a dot). - "There should be exactly one space between function name and parenthesis" errors but all of them are in comments or strings. - "There should be no space before a left square bracket" errors with the same reason why they are false positives. - "Braces should be on a separate line" errors, some of which are in comments and others are one-line functions in our pass classes. The latter seem to be the norm elsewhere and so I left them as they were. I have bootstrapped the two patches on aarch64-linux and bootstrapped and tested them on x86_64-linux. What do you think? Thanks, Martin 2016-12-08 Martin Jambor <mjambor@suse.cz> * omp-general.h: New file. * omp-expand.h: Likewise. * omp-expand.c: Likewise. * omp-device.c: Likewise. * omp-low.h: Include omp-general.h and omp-grid.h. Removed includes of params.h, symbol-summary.h, lto-section-names.h, cilk.h, tree-eh.h, ipa-prop.h, tree-cfgcleanup.h, cfgloop.h, except.h, expr.h, stmt.h, varasm.h, calls.h, explow.h, dojump.h, flags.h, tree-into-ssa.h, tree-cfg.h, cfganal.h, alias.h, emit-rtl.h, optabs.h, expmed.h, alloc-pool.h, cfghooks.h, rtl.h and memmodel.h. (omp_find_combined_for): Declare. (find_omp_clause): Renamed to omp_find_clause and moved to omp-general.h. (free_omp_regions): Renamed to omp_free_regions and moved to omp-expand.h. (replace_oacc_fn_attrib): Renamed to oacc_replace_fn_attrib and moved to omp-general.h. (set_oacc_fn_attrib): Renamed to oacc_set_fn_attrib and moved to omp-general.h. (build_oacc_routine_dims): Renamed to oacc_build_routine_dims and moved to omp-general.h. (get_oacc_fn_attrib): Renamed to oacc_get_fn_attrib and moved to omp-general.h. (oacc_fn_attrib_kernels_p): Moved to omp-general.h. (get_oacc_fn_dim_size): Renamed to oacc_get_fn_dim_size and moved to omp-general.c. (omp_expand_local): Moved to omp-expand.h. (make_gimple_omp_edges): Renamed to omp_make_gimple_edges and moved to omp-expand.h. (omp_finish_file): Moved to omp-device.h. (default_goacc_validate_dims): Renamed to oacc_default_goacc_validate_dims and moved to omp-device.h. (offload_funcs, offload_vars): Moved to omp-device.h. * omp-low.c: Include omp-general.h, omp-device.h and omp-grid.h. (omp_region): Moved to omp-expand.c. (omp_for_data_loop): Moved to omp-general.h. (omp_for_data): Likewise. (oacc_loop): Moved to omp-device.c. (oacc_loop_flags): Moved to omp-general.h. (offload_funcs, offload_vars): Moved to omp-device.c. (root_omp_region): Moved to omp-expand.c. (omp_any_child_fn_dumped): Likewise. (find_omp_clause): Renamed to omp_find_clause and moved to omp-general.c. (is_combined_parallel): kMoved to omp-expand.c. (is_reference): Renamed to omp_is_reference and and moved to omp-general.c. (adjust_for_condition): Renamed to omp_adjust_for_condition and moved to omp-general.c. (get_omp_for_step_from_incr): Renamed to omp_get_for_step_from_incr and moved to omp-general.c. (extract_omp_for_data): Renamed to omp_extract_for_data and moved to omp-general.c. (workshare_safe_to_combine_p): Moved to omp-expand.c. (omp_adjust_chunk_size): Likewise. (get_ws_args_for): Likewise. (get_base_type): Removed. (dump_omp_region): Moved to omp-expand.c. (debug_omp_region): Likewise. (debug_all_omp_regions): Likewise. (new_omp_region): Likewise. (free_omp_region_1): Likewise. (free_omp_regions): Renamed to omp_free_regions and moved to omp-expand.c. (find_combined_for): Renamed to omp_find_combined_for, made global. (build_omp_barrier): Renamed to omp_build_barrier and moved to omp-general.c. (omp_max_vf): Moved to omp-general.c. (omp_max_simt_vf): Likewise. (gimple_build_cond_empty): Moved to omp-expand.c. (parallel_needs_hsa_kernel_p): Likewise. (expand_omp_build_assign): Moved declaration to omp-expand.c. (expand_parallel_call): Moved to omp-expand.c. (expand_cilk_for_call): Likewise. (expand_task_call): Likewise. (vec2chain): Likewise. (remove_exit_barrier): Likewise. (remove_exit_barriers): Likewise. (optimize_omp_library_calls): Likewise. (expand_omp_regimplify_p): Likewise. (expand_omp_build_assign): Likewise. (expand_omp_taskreg): Likewise. (oacc_collapse): Likewise. (expand_oacc_collapse_init): Likewise. (expand_oacc_collapse_vars): Likewise. (expand_omp_for_init_counts): Likewise. (expand_omp_for_init_vars): Likewise. (extract_omp_for_update_vars): Likewise. (expand_omp_ordered_source): Likewise. (expand_omp_ordered_sink): Likewise. (expand_omp_ordered_source_sink): Likewise. (expand_omp_for_ordered_loops): Likewise. (expand_omp_for_generic): Likewise. (expand_omp_for_static_nochunk): Likewise. (find_phi_with_arg_on_edge): Likewise. (expand_omp_for_static_chunk): Likewise. (expand_cilk_for): Likewise. (expand_omp_simd): Likewise. (expand_omp_taskloop_for_outer): Likewise. (expand_omp_taskloop_for_inner): Likewise. (expand_oacc_for): Likewise. (expand_omp_for): Likewise. (expand_omp_sections): Likewise. (expand_omp_single): Likewise. (expand_omp_synch): Likewise. (expand_omp_atomic_load): Likewise. (expand_omp_atomic_store): Likewise. (expand_omp_atomic_fetch_op): Likewise. (expand_omp_atomic_pipeline): Likewise. (expand_omp_atomic_mutex): Likewise. (expand_omp_atomic): Likewise. (oacc_launch_pack): and moved to omp-general.c, made public. (OACC_FN_ATTRIB): Likewise. (replace_oacc_fn_attrib): Renamed to oacc_replace_fn_attrib and moved to omp-general.c. (set_oacc_fn_attrib): Renamed to oacc_set_fn_attrib and moved to omp-general.c. (build_oacc_routine_dims): Renamed to oacc_build_routine_dims and moved to omp-general.c. (get_oacc_fn_attrib): Renamed to oacc_get_fn_attrib and moved to omp-general.c. (oacc_fn_attrib_kernels_p): Moved to omp-general.c. (oacc_fn_attrib_level): Moved to omp-device.c. (get_oacc_fn_dim_size): Renamed to oacc_get_fn_dim_size and moved to omp-general.c. (get_oacc_ifn_dim_arg): Renamed to oacc_get_ifn_dim_arg and moved to omp-general.c. (mark_loops_in_oacc_kernels_region): Moved to omp-expand.c. (grid_launch_attributes_trees): Likewise. (grid_attr_trees): Likewise. (grid_create_kernel_launch_attr_types): Likewise. (grid_insert_store_range_dim): Likewise. (grid_get_kernel_launch_attributes): Likewise. (get_target_argument_identifier_1): Likewise. (get_target_argument_identifier): Likewise. (get_target_argument_value): Likewise. (push_target_argument_according_to_value): Likewise. (get_target_arguments): Likewise. (expand_omp_target): Likewise. (grid_expand_omp_for_loop): Moved to omp-grid.c. (grid_arg_decl_map): Likewise. (grid_remap_kernel_arg_accesses): Likewise. (grid_expand_target_grid_body): Likewise. (expand_omp): Renamed to omp_expand and moved to omp-expand.c. (build_omp_regions_1): Moved to omp-expand.c. (build_omp_regions_root): Likewise. (omp_expand_local): Likewise. (build_omp_regions): Likewise. (execute_expand_omp): Likewise. (pass_data_expand_omp): Likewise. (pass_expand_omp): Likewise. (make_pass_expand_omp): Likewise. (pass_data_expand_omp_ssa): Likewise. (pass_expand_omp_ssa): Likewise. (make_pass_expand_omp_ssa): Likewise. (grid_lastprivate_predicate): Renamed to omp_grid_lastprivate_predicate and moved to omp-grid.c, made public. (grid_prop): Moved to omp-grid.c. (GRID_MISSED_MSG_PREFIX): Likewise. (grid_safe_assignment_p): Likewise. (grid_seq_only_contains_local_assignments): Likewise. (grid_find_single_omp_among_assignments_1): Likewise. (grid_find_single_omp_among_assignments): Likewise. (grid_find_ungridifiable_statement): Likewise. (grid_parallel_clauses_gridifiable): Likewise. (grid_inner_loop_gridifiable_p): Likewise. (grid_dist_follows_simple_pattern): Likewise. (grid_gfor_follows_tiling_pattern): Likewise. (grid_call_permissible_in_distribute_p): Likewise. (grid_handle_call_in_distribute): Likewise. (grid_dist_follows_tiling_pattern): Likewise. (grid_target_follows_gridifiable_pattern): Likewise. (grid_remap_prebody_decls): Likewise. (grid_var_segment): Likewise. (grid_mark_variable_segment): Likewise. (grid_copy_leading_local_assignments): Likewise. (grid_process_grid_body): Likewise. (grid_eliminate_combined_simd_part): Likewise. (grid_mark_tiling_loops): Likewise. (grid_mark_tiling_parallels_and_loops): Likewise. (grid_process_kernel_body_copy): Likewise. (grid_attempt_target_gridification): Likewise. (grid_gridify_all_targets_stmt): Likewise. (grid_gridify_all_targets): Renamed to omp_grid_gridify_all_targets and moved to omp-grid.c, made public. (make_gimple_omp_edges): Renamed to omp_make_gimple_edges and moved to omp-expand.c. (add_decls_addresses_to_decl_constructor): Moved to omp-device.c. (omp_finish_file): Likewise. (oacc_thread_numbers): Likewise. (oacc_xform_loop): Likewise. (oacc_default_dims, oacc_min_dims): Likewise. (oacc_parse_default_dims): Likewise. (oacc_validate_dims): Likewise. (new_oacc_loop_raw): Likewise. (new_oacc_loop_outer): Likewise. (new_oacc_loop): Likewise. (new_oacc_loop_routine): Likewise. (finish_oacc_loop): Likewise. (free_oacc_loop): Likewise. (dump_oacc_loop_part): Likewise. (dump_oacc_loop): Likewise. (debug_oacc_loop): Likewise. (oacc_loop_discover_walk): Likewise. (oacc_loop_sibling_nreverse): Likewise. (oacc_loop_discovery): Likewise. (oacc_loop_xform_head_tail): Likewise. (oacc_loop_xform_loop): Likewise. (oacc_loop_process): Likewise. (oacc_loop_fixed_partitions): Likewise. (oacc_loop_auto_partitions): Likewise. (oacc_loop_partition): Likewise. (default_goacc_fork_join): Likewise. (default_goacc_reduction): Likewise. (execute_oacc_device_lower): Likewise. (default_goacc_validate_dims): Likewise. (default_goacc_dim_limit): Likewise. (pass_data_oacc_device_lower): Likewise. (pass_oacc_device_lower): Likewise. (make_pass_oacc_device_lower): Likewise. (execute_omp_device_lower): Likewise. (pass_data_omp_device_lower): Likewise. (pass_omp_device_lower): Likewise. (make_pass_omp_device_lower): Likewise. (pass_data_omp_target_link): Likewise. (pass_omp_target_link): Likewise. (find_link_var_op): Likewise. (pass_omp_target_link::execute): Likewise. (make_pass_omp_target_link): Likewise. * Makefile.in (OBJS): Added omp-device.o, omp-expand.o, omp-general.o and omp-grid.o. (GTFILES): Added omp-device.h, omp-device.c and omp-expand.c, removed omp-low.h. * gimple-fold.c: Include omp-general.h instead of omp-low.h. (fold_internal_goacc_dim): Adjusted calls to get_oacc_ifn_dim_arg and get_oacc_fn_dim_size to use their new names. * gimplify.c: Include omp-low.h. (omp_notice_variable): Adjust the call to get_oacc_fn_attrib to use its new name. (gimplify_omp_task): Adjusted calls to find_omp_clause to use its new name. (gimplify_omp_for): Likewise. * lto-cgraph.c: Include omp-device.h instead of omp-low.h. * toplev.c: Include omp-device.h instead of omp-low.h. * tree-cfg.c: Include omp-general.h instead of omp-low.h. Also include omp-expand.h. (make_edges_bb): Adjusted the call to make_gimple_omp_edges to use its new name. (make_edges): Adjust the call to free_omp_regions to use its new name. * tree-parloops.c: Include omp-general.h. (create_parallel_loop): Adjusted the call to set_oacc_fn_attrib to use its new name. (parallelize_loops): Adjusted the call to get_oacc_fn_attrib to use its new name. * tree-ssa-loop.c: Include omp-general.h instead of omp-low.h. (gate_oacc_kernels): Adjusted the call to get_oacc_fn_attrib to use its new name. * tree-vrp.c: Include omp-general.h instead of omp-low.h. (extract_range_basic): Adjusted calls to get_oacc_ifn_dim_arg and get_oacc_fn_dim_size to use their new names. * varpool.c: Include omp-device.h instead of omp-low.h. * gengtype.c (open_base_files): Replace omp-low.h with omp-device.h in ifiles. * config/nvptx/nvptx.c: Include omp-generic.c. (nvptx_expand_call): Adjusted the call to get_oacc_fn_attrib to use its new name. (nvptx_reorg): Likewise. (nvptx_record_offload_symbol): Likewise. gcc/c-family: * c-omp.c: Include omp-general.h instead of omp-low.h. (c_finish_oacc_wait): Adjusted call to find_omp_clause to use its new name. gcc/c/ * c-parser.c: Include omp-general.h and omp-device.h instead of omp-low.h. (c_finish_oacc_routine): Adjusted call to get_oacc_fn_attrib, build_oacc_routine_dims and replace_oacc_fn_attrib to use their new names. (c_parser_oacc_enter_exit_data): Adjusted call to find_omp_clause to use its new name. (c_parser_oacc_update): Likewise. (c_parser_omp_simd): Likewise. (c_parser_omp_target_update): Likewise. * c-typeck.c: Include omp-general.h instead of omp-low.h. (c_finish_omp_cancel): Adjusted call to find_omp_clause to use its new name. (c_finish_omp_cancellation_point): Likewise. * gimple-parser.c: Do not include omp-low.h gcc/cp/ * parser.c: Include omp-general.h and omp-device.h instead of omp-low.h. (cp_parser_omp_simd): Adjusted calls to find_omp_clause to use its new name. (cp_parser_omp_target_update): Likewise. (cp_parser_oacc_declare): Likewise. (cp_parser_oacc_enter_exit_data): Likewise. (cp_parser_oacc_update): Likewise. (cp_finalize_oacc_routine): Adjusted call to get_oacc_fn_attrib, build_oacc_routine_dims and replace_oacc_fn_attrib to use their new names. * semantics.c: Include omp-general insteda of omp-low.h. (finish_omp_for): Adjusted calls to find_omp_clause to use its new name. (finish_omp_cancel): Likewise. (finish_omp_cancellation_point): Likewise. fortran/ * trans-openmp.c: Include omp-general.h. 2016-12-08 Martin Jambor <mjambor@suse.cz> * omp-device.c: Fix coding style. * omp-expand.c: Likewise. * omp-general.c: Likewise. * omp-grid.c: Likewise. * omp-low.c: Fix coding style of parts touched by the previous splitting patch. [-- Attachment #2: 0001-Split-omp-low-into-multiple-files.patch.gz --] [-- Type: application/x-gzip, Size: 187077 bytes --] [-- Attachment #3: 0002-Coding-style-fixes.patch.gz --] [-- Type: application/x-gzip, Size: 9024 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-13 10:16 ` Martin Jambor @ 2016-12-13 10:35 ` Jakub Jelinek 2016-12-14 16:56 ` Martin Jambor 2016-12-13 12:58 ` Alexander Monakov 1 sibling, 1 reply; 18+ messages in thread From: Jakub Jelinek @ 2016-12-13 10:35 UTC (permalink / raw) To: Alexander Monakov, GCC Patches, Cesar Philippidis, Thomas Schwinge On Tue, Dec 13, 2016 at 11:15:43AM +0100, Martin Jambor wrote: > I have bootstrapped the two patches on aarch64-linux and bootstrapped > and tested them on x86_64-linux. What do you think? Thanks a lot for the work. If you wouldn't mind doing a couple of further changes (see below), I'd appreciate it, but if you want to commit it right away, I'm ok with that too. @@ -4321,7 +4322,7 @@ expand_cilk_for (struct omp_region *region, struct omp_for_data *fd) tree child_fndecl = gimple_omp_parallel_child_fn ( - as_a <gomp_parallel *> (last_stmt (region->outer->entry))); + as_a <gomp_parallel *> (last_stmt (region->outer->entry))); tree t, low_val = NULL_TREE, high_val = NULL_TREE; for (t = DECL_ARGUMENTS (child_fndecl); t; t = TREE_CHAIN (t)) { My preference for the above would be gomp_parallel *par_stmt = as_a <gomp_parallel *> (last_stmt (region->outer->entry)); tree child_fndecl = gimple_omp_parallel_child_fn (par_stmt); @@ -6428,7 +6429,7 @@ expand_omp_atomic_pipeline (basic_block load_bb, basic_block store_bb, floating point. This allows the atomic operation to properly succeed even with NaNs and -0.0. */ stmt = gimple_build_cond_empty - (build2 (NE_EXPR, boolean_type_node, + (build2 (NE_EXPR, boolean_type_node, new_storedi, old_vali)); gsi_insert_before (&si, stmt, GSI_SAME_STMT); And here tree ne = build2 (NE_EXPR, boolean_type_node, new_storedi, old_vali); stmt = gimple_build_cond_empty (ne); @@ -442,7 +442,7 @@ omp_max_simt_vf (void) if (!optimize) return 0; if (ENABLE_OFFLOADING) - for (const char *c = getenv ("OFFLOAD_TARGET_NAMES"); c; ) + for (const char *c = getenv ("OFFLOAD_TARGET_NAMES"); c;) I admit I don't know what the coding style we have in this case. Ok either way. @@ -5860,7 +5860,7 @@ lower_omp_sections (gimple_stmt_iterator *gsi_p, omp_context *ctx) new_body = maybe_catch_exception (new_body); t = gimple_build_omp_return - (!!omp_find_clause (gimple_omp_sections_clauses (stmt), + (!!omp_find_clause (gimple_omp_sections_clauses (stmt), OMP_CLAUSE_NOWAIT)); gimple_seq_add_stmt (&new_body, t); maybe_add_implicit_barrier_cancel (ctx, &new_body); @@ -6023,7 +6023,7 @@ lower_omp_single (gimple_stmt_iterator *gsi_p, omp_context *ctx) bind_body = maybe_catch_exception (bind_body); t = gimple_build_omp_return - (!!omp_find_clause (gimple_omp_single_clauses (single_stmt), + (!!omp_find_clause (gimple_omp_single_clauses (single_stmt), OMP_CLAUSE_NOWAIT)); gimple_seq_add_stmt (&bind_body_tail, t); maybe_add_implicit_barrier_cancel (ctx, &bind_body_tail); And in the above 2 spots something like: bool nowait = omp_find_clause (gimple_omp_single_clauses (single_stmt), OMP_CLAUSE_NOWAIT) != NULL_TREE; t = gimple_build_omp_return (nowait); (wonder why we use t for gimple, renaming t var to g would be also nice). But ok either way. @@ -8668,7 +8669,7 @@ lower_omp_1 (gimple_stmt_iterator *gsi_p, omp_context *ctx) break; case GIMPLE_TRANSACTION: lower_omp (gimple_transaction_body_ptr ( - as_a <gtransaction *> (stmt)), + as_a <gtransaction *> (stmt)), ctx); Here it fits, so no need to wrap: lower_omp (gimple_transaction_body_ptr (as_a <gtransaction *> (stmt)), ctx); Jakub ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-13 10:35 ` Jakub Jelinek @ 2016-12-14 16:56 ` Martin Jambor 0 siblings, 0 replies; 18+ messages in thread From: Martin Jambor @ 2016-12-14 16:56 UTC (permalink / raw) To: Jakub Jelinek Cc: Alexander Monakov, GCC Patches, Cesar Philippidis, Thomas Schwinge [-- Attachment #1: Type: text/plain, Size: 18035 bytes --] Hi, On Tue, Dec 13, 2016 at 11:35:37AM +0100, Jakub Jelinek wrote: > On Tue, Dec 13, 2016 at 11:15:43AM +0100, Martin Jambor wrote: > > I have bootstrapped the two patches on aarch64-linux and bootstrapped > > and tested them on x86_64-linux. What do you think? > > Thanks a lot for the work. If you wouldn't mind doing a couple of further > changes (see below), I'd appreciate it, but if you want to commit it right > away, I'm ok with that too. I have rebased the patches on current trunk and: 1) updated the first patch to rename omp-device.[ch] to omp-offload.[ch] as suggested by Thomas and fixed the mistakes in ChangeLog discovered by Alex, and 2) updated the second patch to deal with most of the issues below and to remove spaces before tabs that Alex found. I am bootstrapping the result now and because they have been approved and since it seems that a number of people have had a chance to look at and even test them, I will commit them after the bootstrap and testing finishes. The updated patches are attached, ChangeLogs are at the end of this message. > > @@ -4321,7 +4322,7 @@ expand_cilk_for (struct omp_region *region, struct omp_for_data *fd) > > tree child_fndecl > = gimple_omp_parallel_child_fn ( > - as_a <gomp_parallel *> (last_stmt (region->outer->entry))); > + as_a <gomp_parallel *> (last_stmt (region->outer->entry))); > tree t, low_val = NULL_TREE, high_val = NULL_TREE; > for (t = DECL_ARGUMENTS (child_fndecl); t; t = TREE_CHAIN (t)) > { > > My preference for the above would be > > gomp_parallel *par_stmt > = as_a <gomp_parallel *> (last_stmt (region->outer->entry)); > tree child_fndecl = gimple_omp_parallel_child_fn (par_stmt); OK, I have changed it so. > > @@ -6428,7 +6429,7 @@ expand_omp_atomic_pipeline (basic_block load_bb, basic_block store_bb, > floating point. This allows the atomic operation to properly > succeed even with NaNs and -0.0. */ > stmt = gimple_build_cond_empty > - (build2 (NE_EXPR, boolean_type_node, > + (build2 (NE_EXPR, boolean_type_node, > new_storedi, old_vali)); > gsi_insert_before (&si, stmt, GSI_SAME_STMT); > > And here > > tree ne = build2 (NE_EXPR, boolean_type_node, new_storedi, old_vali); > stmt = gimple_build_cond_empty (ne); Likewise. > > @@ -442,7 +442,7 @@ omp_max_simt_vf (void) > if (!optimize) > return 0; > if (ENABLE_OFFLOADING) > - for (const char *c = getenv ("OFFLOAD_TARGET_NAMES"); c; ) > + for (const char *c = getenv ("OFFLOAD_TARGET_NAMES"); c;) > > I admit I don't know what the coding style we have in this case. Ok > either way. I have made this change in order to silence a complaint from check_GNU_style.sh. I think the only sane way to write the above is as a while loop :-) But I have left it as it was, I really wanted to be changing things only slightly. > > @@ -5860,7 +5860,7 @@ lower_omp_sections (gimple_stmt_iterator *gsi_p, omp_context *ctx) > new_body = maybe_catch_exception (new_body); > > t = gimple_build_omp_return > - (!!omp_find_clause (gimple_omp_sections_clauses (stmt), > + (!!omp_find_clause (gimple_omp_sections_clauses (stmt), > OMP_CLAUSE_NOWAIT)); > gimple_seq_add_stmt (&new_body, t); > maybe_add_implicit_barrier_cancel (ctx, &new_body); > @@ -6023,7 +6023,7 @@ lower_omp_single (gimple_stmt_iterator *gsi_p, omp_context *ctx) > bind_body = maybe_catch_exception (bind_body); > > t = gimple_build_omp_return > - (!!omp_find_clause (gimple_omp_single_clauses (single_stmt), > + (!!omp_find_clause (gimple_omp_single_clauses (single_stmt), > OMP_CLAUSE_NOWAIT)); > gimple_seq_add_stmt (&bind_body_tail, t); > maybe_add_implicit_barrier_cancel (ctx, &bind_body_tail); > > And in the above 2 spots something like: > bool nowait = omp_find_clause (gimple_omp_single_clauses (single_stmt), > OMP_CLAUSE_NOWAIT) != NULL_TREE; > t = gimple_build_omp_return (nowait); I have changed it so too. > > (wonder why we use t for gimple, renaming t var to g would be also nice). But > ok either way. I have changed the second occurrence to use g because it was the only use of t, but left t in the first one because it had other uses. > > @@ -8668,7 +8669,7 @@ lower_omp_1 (gimple_stmt_iterator *gsi_p, omp_context *ctx) > break; > case GIMPLE_TRANSACTION: > lower_omp (gimple_transaction_body_ptr ( > - as_a <gtransaction *> (stmt)), > + as_a <gtransaction *> (stmt)), > ctx); > > Here it fits, so no need to wrap: > lower_omp (gimple_transaction_body_ptr (as_a <gtransaction *> (stmt)), > ctx); > > I have changed this as you requested too. Thanks, Martin 2016-12-14 Martin Jambor <mjambor@suse.cz> * omp-general.h: New file. * omp-general.c: New file. * omp-expand.h: Likewise. * omp-expand.c: Likewise. * omp-offload.h: Likewise. * omp-offload.c: Likewise. * omp-grid.c: Likewise. * omp-grid.c: Likewise. * omp-low.h: Include omp-general.h and omp-grid.h. Removed includes of params.h, symbol-summary.h, lto-section-names.h, cilk.h, tree-eh.h, ipa-prop.h, tree-cfgcleanup.h, cfgloop.h, except.h, expr.h, stmt.h, varasm.h, calls.h, explow.h, dojump.h, flags.h, tree-into-ssa.h, tree-cfg.h, cfganal.h, alias.h, emit-rtl.h, optabs.h, expmed.h, alloc-pool.h, cfghooks.h, rtl.h and memmodel.h. (omp_find_combined_for): Declare. (find_omp_clause): Renamed to omp_find_clause and moved to omp-general.h. (free_omp_regions): Renamed to omp_free_regions and moved to omp-expand.h. (replace_oacc_fn_attrib): Renamed to oacc_replace_fn_attrib and moved to omp-general.h. (set_oacc_fn_attrib): Renamed to oacc_set_fn_attrib and moved to omp-general.h. (build_oacc_routine_dims): Renamed to oacc_build_routine_dims and moved to omp-general.h. (get_oacc_fn_attrib): Renamed to oacc_get_fn_attrib and moved to omp-general.h. (oacc_fn_attrib_kernels_p): Moved to omp-general.h. (get_oacc_fn_dim_size): Renamed to oacc_get_fn_dim_size and moved to omp-general.c. (omp_expand_local): Moved to omp-expand.h. (make_gimple_omp_edges): Renamed to omp_make_gimple_edges and moved to omp-expand.h. (omp_finish_file): Moved to omp-offload.h. (default_goacc_validate_dims): Renamed to oacc_default_goacc_validate_dims and moved to omp-offload.h. (offload_funcs, offload_vars): Moved to omp-offload.h. * omp-low.c: Include omp-general.h, omp-offload.h and omp-grid.h. (omp_region): Moved to omp-expand.c. (omp_for_data_loop): Moved to omp-general.h. (omp_for_data): Likewise. (oacc_loop): Moved to omp-offload.c. (oacc_loop_flags): Moved to omp-general.h. (offload_funcs, offload_vars): Moved to omp-offload.c. (root_omp_region): Moved to omp-expand.c. (omp_any_child_fn_dumped): Likewise. (find_omp_clause): Renamed to omp_find_clause and moved to omp-general.c. (is_combined_parallel): Moved to omp-expand.c. (is_reference): Renamed to omp_is_reference and and moved to omp-general.c. (adjust_for_condition): Renamed to omp_adjust_for_condition and moved to omp-general.c. (get_omp_for_step_from_incr): Renamed to omp_get_for_step_from_incr and moved to omp-general.c. (extract_omp_for_data): Renamed to omp_extract_for_data and moved to omp-general.c. (workshare_safe_to_combine_p): Moved to omp-expand.c. (omp_adjust_chunk_size): Likewise. (get_ws_args_for): Likewise. (get_base_type): Removed. (dump_omp_region): Moved to omp-expand.c. (debug_omp_region): Likewise. (debug_all_omp_regions): Likewise. (new_omp_region): Likewise. (free_omp_region_1): Likewise. (free_omp_regions): Renamed to omp_free_regions and moved to omp-expand.c. (find_combined_for): Renamed to omp_find_combined_for, made global. (build_omp_barrier): Renamed to omp_build_barrier and moved to omp-general.c. (omp_max_vf): Moved to omp-general.c. (omp_max_simt_vf): Likewise. (gimple_build_cond_empty): Moved to omp-expand.c. (parallel_needs_hsa_kernel_p): Likewise. (expand_omp_build_assign): Moved declaration to omp-expand.c. (expand_parallel_call): Moved to omp-expand.c. (expand_cilk_for_call): Likewise. (expand_task_call): Likewise. (vec2chain): Likewise. (remove_exit_barrier): Likewise. (remove_exit_barriers): Likewise. (optimize_omp_library_calls): Likewise. (expand_omp_regimplify_p): Likewise. (expand_omp_build_assign): Likewise. (expand_omp_taskreg): Likewise. (oacc_collapse): Likewise. (expand_oacc_collapse_init): Likewise. (expand_oacc_collapse_vars): Likewise. (expand_omp_for_init_counts): Likewise. (expand_omp_for_init_vars): Likewise. (extract_omp_for_update_vars): Likewise. (expand_omp_ordered_source): Likewise. (expand_omp_ordered_sink): Likewise. (expand_omp_ordered_source_sink): Likewise. (expand_omp_for_ordered_loops): Likewise. (expand_omp_for_generic): Likewise. (expand_omp_for_static_nochunk): Likewise. (find_phi_with_arg_on_edge): Likewise. (expand_omp_for_static_chunk): Likewise. (expand_cilk_for): Likewise. (expand_omp_simd): Likewise. (expand_omp_taskloop_for_outer): Likewise. (expand_omp_taskloop_for_inner): Likewise. (expand_oacc_for): Likewise. (expand_omp_for): Likewise. (expand_omp_sections): Likewise. (expand_omp_single): Likewise. (expand_omp_synch): Likewise. (expand_omp_atomic_load): Likewise. (expand_omp_atomic_store): Likewise. (expand_omp_atomic_fetch_op): Likewise. (expand_omp_atomic_pipeline): Likewise. (expand_omp_atomic_mutex): Likewise. (expand_omp_atomic): Likewise. (oacc_launch_pack): and moved to omp-general.c, made public. (OACC_FN_ATTRIB): Likewise. (replace_oacc_fn_attrib): Renamed to oacc_replace_fn_attrib and moved to omp-general.c. (set_oacc_fn_attrib): Renamed to oacc_set_fn_attrib and moved to omp-general.c. (build_oacc_routine_dims): Renamed to oacc_build_routine_dims and moved to omp-general.c. (get_oacc_fn_attrib): Renamed to oacc_get_fn_attrib and moved to omp-general.c. (oacc_fn_attrib_kernels_p): Moved to omp-general.c. (oacc_fn_attrib_level): Moved to omp-offload.c. (get_oacc_fn_dim_size): Renamed to oacc_get_fn_dim_size and moved to omp-general.c. (get_oacc_ifn_dim_arg): Renamed to oacc_get_ifn_dim_arg and moved to omp-general.c. (mark_loops_in_oacc_kernels_region): Moved to omp-expand.c. (grid_launch_attributes_trees): Likewise. (grid_attr_trees): Likewise. (grid_create_kernel_launch_attr_types): Likewise. (grid_insert_store_range_dim): Likewise. (grid_get_kernel_launch_attributes): Likewise. (get_target_argument_identifier_1): Likewise. (get_target_argument_identifier): Likewise. (get_target_argument_value): Likewise. (push_target_argument_according_to_value): Likewise. (get_target_arguments): Likewise. (expand_omp_target): Likewise. (grid_expand_omp_for_loop): Moved to omp-grid.c. (grid_arg_decl_map): Likewise. (grid_remap_kernel_arg_accesses): Likewise. (grid_expand_target_grid_body): Likewise. (expand_omp): Renamed to omp_expand and moved to omp-expand.c. (build_omp_regions_1): Moved to omp-expand.c. (build_omp_regions_root): Likewise. (omp_expand_local): Likewise. (build_omp_regions): Likewise. (execute_expand_omp): Likewise. (pass_data_expand_omp): Likewise. (pass_expand_omp): Likewise. (make_pass_expand_omp): Likewise. (pass_data_expand_omp_ssa): Likewise. (pass_expand_omp_ssa): Likewise. (make_pass_expand_omp_ssa): Likewise. (grid_lastprivate_predicate): Renamed to omp_grid_lastprivate_predicate and moved to omp-grid.c, made public. (grid_prop): Moved to omp-grid.c. (GRID_MISSED_MSG_PREFIX): Likewise. (grid_safe_assignment_p): Likewise. (grid_seq_only_contains_local_assignments): Likewise. (grid_find_single_omp_among_assignments_1): Likewise. (grid_find_single_omp_among_assignments): Likewise. (grid_find_ungridifiable_statement): Likewise. (grid_parallel_clauses_gridifiable): Likewise. (grid_inner_loop_gridifiable_p): Likewise. (grid_dist_follows_simple_pattern): Likewise. (grid_gfor_follows_tiling_pattern): Likewise. (grid_call_permissible_in_distribute_p): Likewise. (grid_handle_call_in_distribute): Likewise. (grid_dist_follows_tiling_pattern): Likewise. (grid_target_follows_gridifiable_pattern): Likewise. (grid_remap_prebody_decls): Likewise. (grid_var_segment): Likewise. (grid_mark_variable_segment): Likewise. (grid_copy_leading_local_assignments): Likewise. (grid_process_grid_body): Likewise. (grid_eliminate_combined_simd_part): Likewise. (grid_mark_tiling_loops): Likewise. (grid_mark_tiling_parallels_and_loops): Likewise. (grid_process_kernel_body_copy): Likewise. (grid_attempt_target_gridification): Likewise. (grid_gridify_all_targets_stmt): Likewise. (grid_gridify_all_targets): Renamed to omp_grid_gridify_all_targets and moved to omp-grid.c, made public. (make_gimple_omp_edges): Renamed to omp_make_gimple_edges and moved to omp-expand.c. (add_decls_addresses_to_decl_constructor): Moved to omp-offload.c. (omp_finish_file): Likewise. (oacc_thread_numbers): Likewise. (oacc_xform_loop): Likewise. (oacc_default_dims, oacc_min_dims): Likewise. (oacc_parse_default_dims): Likewise. (oacc_validate_dims): Likewise. (new_oacc_loop_raw): Likewise. (new_oacc_loop_outer): Likewise. (new_oacc_loop): Likewise. (new_oacc_loop_routine): Likewise. (finish_oacc_loop): Likewise. (free_oacc_loop): Likewise. (dump_oacc_loop_part): Likewise. (dump_oacc_loop): Likewise. (debug_oacc_loop): Likewise. (oacc_loop_discover_walk): Likewise. (oacc_loop_sibling_nreverse): Likewise. (oacc_loop_discovery): Likewise. (oacc_loop_xform_head_tail): Likewise. (oacc_loop_xform_loop): Likewise. (oacc_loop_process): Likewise. (oacc_loop_fixed_partitions): Likewise. (oacc_loop_auto_partitions): Likewise. (oacc_loop_partition): Likewise. (default_goacc_fork_join): Likewise. (default_goacc_reduction): Likewise. (execute_oacc_device_lower): Likewise. (default_goacc_validate_dims): Likewise. (default_goacc_dim_limit): Likewise. (pass_data_oacc_device_lower): Likewise. (pass_oacc_device_lower): Likewise. (make_pass_oacc_device_lower): Likewise. (execute_omp_device_lower): Likewise. (pass_data_omp_device_lower): Likewise. (pass_omp_device_lower): Likewise. (make_pass_omp_device_lower): Likewise. (pass_data_omp_target_link): Likewise. (pass_omp_target_link): Likewise. (find_link_var_op): Likewise. (pass_omp_target_link::execute): Likewise. (make_pass_omp_target_link): Likewise. * Makefile.in (OBJS): Added omp-offload.o, omp-expand.o, omp-general.o and omp-grid.o. (GTFILES): Added omp-offload.h, omp-offload.c and omp-expand.c, removed omp-low.h. * gimple-fold.c: Include omp-general.h instead of omp-low.h. (fold_internal_goacc_dim): Adjusted calls to get_oacc_ifn_dim_arg and get_oacc_fn_dim_size to use their new names. * gimplify.c: Include omp-low.h. (omp_notice_variable): Adjust the call to get_oacc_fn_attrib to use its new name. (gimplify_omp_task): Adjusted calls to find_omp_clause to use its new name. (gimplify_omp_for): Likewise. * lto-cgraph.c: Include omp-offload.h instead of omp-low.h. * toplev.c: Include omp-offload.h instead of omp-low.h. * tree-cfg.c: Include omp-general.h instead of omp-low.h. Also include omp-expand.h. (make_edges_bb): Adjusted the call to make_gimple_omp_edges to use its new name. (make_edges): Adjust the call to free_omp_regions to use its new name. * tree-parloops.c: Include omp-general.h. (create_parallel_loop): Adjusted the call to set_oacc_fn_attrib to use its new name. (parallelize_loops): Adjusted the call to get_oacc_fn_attrib to use its new name. * tree-ssa-loop.c: Include omp-general.h instead of omp-low.h. (gate_oacc_kernels): Adjusted the call to get_oacc_fn_attrib to use its new name. * tree-vrp.c: Include omp-general.h instead of omp-low.h. (extract_range_basic): Adjusted calls to get_oacc_ifn_dim_arg and get_oacc_fn_dim_size to use their new names. * varpool.c: Include omp-offload.h instead of omp-low.h. * gengtype.c (open_base_files): Replace omp-low.h with omp-offload.h in ifiles. * config/nvptx/nvptx.c: Include omp-general.c. (nvptx_expand_call): Adjusted the call to get_oacc_fn_attrib to use its new name. (nvptx_reorg): Likewise. (nvptx_record_offload_symbol): Likewise. gcc/c-family: * c-omp.c: Include omp-general.h instead of omp-low.h. (c_finish_oacc_wait): Adjusted call to find_omp_clause to use its new name. gcc/c/ * c-parser.c: Include omp-general.h and omp-offload.h instead of omp-low.h. (c_finish_oacc_routine): Adjusted call to get_oacc_fn_attrib, build_oacc_routine_dims and replace_oacc_fn_attrib to use their new names. (c_parser_oacc_enter_exit_data): Adjusted call to find_omp_clause to use its new name. (c_parser_oacc_update): Likewise. (c_parser_omp_simd): Likewise. (c_parser_omp_target_update): Likewise. * c-typeck.c: Include omp-general.h instead of omp-low.h. (c_finish_omp_cancel): Adjusted call to find_omp_clause to use its new name. (c_finish_omp_cancellation_point): Likewise. * gimple-parser.c: Do not include omp-low.h gcc/cp/ * parser.c: Include omp-general.h and omp-offload.h instead of omp-low.h. (cp_parser_omp_simd): Adjusted calls to find_omp_clause to use its new name. (cp_parser_omp_target_update): Likewise. (cp_parser_oacc_declare): Likewise. (cp_parser_oacc_enter_exit_data): Likewise. (cp_parser_oacc_update): Likewise. (cp_finalize_oacc_routine): Adjusted call to get_oacc_fn_attrib, build_oacc_routine_dims and replace_oacc_fn_attrib to use their new names. * semantics.c: Include omp-general insteda of omp-low.h. (finish_omp_for): Adjusted calls to find_omp_clause to use its new name. (finish_omp_cancel): Likewise. (finish_omp_cancellation_point): Likewise. fortran/ * trans-openmp.c: Include omp-general.h. 2016-12-14 Martin Jambor <mjambor@suse.cz> * omp-device.c: Fix coding style. * omp-expand.c: Likewise. * omp-general.c: Likewise. * omp-grid.c: Likewise. * omp-low.c: Fix coding style of parts touched by the previous splitting patch. [-- Attachment #2: 0001-Split-omp-low-into-multiple-files.patch.gz --] [-- Type: application/x-gzip, Size: 187474 bytes --] [-- Attachment #3: 0002-Coding-style-fixes.patch.gz --] [-- Type: application/x-gzip, Size: 9877 bytes --] ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-13 10:16 ` Martin Jambor 2016-12-13 10:35 ` Jakub Jelinek @ 2016-12-13 12:58 ` Alexander Monakov 1 sibling, 0 replies; 18+ messages in thread From: Alexander Monakov @ 2016-12-13 12:58 UTC (permalink / raw) To: Martin Jambor Cc: Jakub Jelinek, GCC Patches, Cesar Philippidis, Thomas Schwinge On Tue, 13 Dec 2016, Martin Jambor wrote: > I have bootstrapped the two patches on aarch64-linux and bootstrapped > and tested them on x86_64-linux. What do you think? Sorry for my 'false alarm' about cp/parser.c conflict in the previous mail -- I thought I was applying your patch to trunk, but now I see my tree was outdated. In the new patch there are 7 whitespace issues that git complains about: zcat 0001-Split-omp-low-into-multiple-files.patch.gz | git apply --check --whitespace=error-all - <stdin>:2779: space before tab in indent. # BLOCK 2 (PAR_ENTRY_BB) <stdin>:5391: space before tab in indent. true, GSI_SAME_STMT); <stdin>:8174: space before tab in indent. after a stmt, not before. */ <stdin>:9105: space before tab in indent. GOMP_atomic_start (); <stdin>:9106: space before tab in indent. *addr = rhs; <stdin>:9107: space before tab in indent. GOMP_atomic_end (); <stdin>:10327: space before tab in indent. region. */ error: 7 lines add whitespace errors. A couple of typos in the Changelog: > (is_combined_parallel): kMoved to omp-expand.c. s/k// > * config/nvptx/nvptx.c: Include omp-generic.c. omp-general.h :) Alexander ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-09 13:08 [PATCH] omp-low.c split Martin Jambor 2016-12-09 13:25 ` Alexander Monakov @ 2016-12-13 11:39 ` Thomas Schwinge 2016-12-13 11:43 ` Jakub Jelinek 2021-08-04 12:40 ` Thomas Schwinge 2 siblings, 1 reply; 18+ messages in thread From: Thomas Schwinge @ 2016-12-13 11:39 UTC (permalink / raw) To: Martin Jambor Cc: Jakub Jelinek, Cesar Philippidis, Alexander Monakov, GCC Patches Hi! On Fri, 9 Dec 2016 14:08:21 +0100, Martin Jambor <mjambor@suse.cz> wrote: > this is the promised attempt at splitting omp-low.c [...] Yay! \o/ I have not yet had a chance to review/test this patch, but I plan to. A few initial comments from the "bike shed departement"; I understand in GCC sources it will not be easy to rename stuff (such as files) later, so we should get the names agreed upon early: Generally, I agree with your division of "omp-low.c" parts. > - move everything that is part of pass_oacc_device_lower, > pass_omp_device_lower and pass_omp_target_link to a new file > omp-device.h, Should we call this file "omp-offload.c", as offloading is what this deals with, is the term we agreed to generally use (as far as I can tell)? > - move all pre-lowering gridification stuff to a new file > omp-grid.c. [...] Is that code generic enough to not call this file "omp-hsa.c" or similar? > - I moved stuff that was used from all over the place to a new file > omp-general.c (unless it would mean exposing omp_region or > omp_context types). I'd have called that simply "omp.c". > I am opened to suggestions what to do differently, names of the file > are for example of course subject to discussion, and I absolutely > welcome any review and checking, for one I am not going to pretend I > understand the stuff I put into omp-device.c. If however there is > consensus that we should do something like this, I would like to ask > the community to freeze omp-low.c file until this gets committed, I > hope you understand that I am afraid of any conflicts. I very much understand... :-| When I had worked on the very same thing months ago, and my changes went without review/approval for a long time, I had spent numerous hours on keeping my patch up to date. So, I'm happy to see that this is now near approval! (Even though I don't understand what's different now from when I worked on the same thing back then...) I hope to have time later today to review/test your actual patch. Grüße Thomas ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-13 11:39 ` Thomas Schwinge @ 2016-12-13 11:43 ` Jakub Jelinek 2016-12-13 12:42 ` Martin Jambor 0 siblings, 1 reply; 18+ messages in thread From: Jakub Jelinek @ 2016-12-13 11:43 UTC (permalink / raw) To: Thomas Schwinge Cc: Martin Jambor, Cesar Philippidis, Alexander Monakov, GCC Patches On Tue, Dec 13, 2016 at 12:39:01PM +0100, Thomas Schwinge wrote: > On Fri, 9 Dec 2016 14:08:21 +0100, Martin Jambor <mjambor@suse.cz> wrote: > > this is the promised attempt at splitting omp-low.c [...] > > Yay! \o/ > > I have not yet had a chance to review/test this patch, but I plan to. > > A few initial comments from the "bike shed departement"; I understand in > GCC sources it will not be easy to rename stuff (such as files) later, so > we should get the names agreed upon early: > > Generally, I agree with your division of "omp-low.c" parts. > > > - move everything that is part of pass_oacc_device_lower, > > pass_omp_device_lower and pass_omp_target_link to a new file > > omp-device.h, > > Should we call this file "omp-offload.c", as offloading is what this > deals with, is the term we agreed to generally use (as far as I can > tell)? That would be fine with me too. > > - move all pre-lowering gridification stuff to a new file > > omp-grid.c. [...] > > Is that code generic enough to not call this file "omp-hsa.c" or similar? And this as well. But omp-grid.c is fine too. > > - I moved stuff that was used from all over the place to a new file > > omp-general.c (unless it would mean exposing omp_region or > > omp_context types). > > I'd have called that simply "omp.c". The problem with that is that the corresponding header can't be called omp.h for obvious reasons, we already have one with very different meaning. Jakub ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-13 11:43 ` Jakub Jelinek @ 2016-12-13 12:42 ` Martin Jambor 2016-12-13 14:48 ` Cesar Philippidis 2016-12-14 13:16 ` Thomas Schwinge 0 siblings, 2 replies; 18+ messages in thread From: Martin Jambor @ 2016-12-13 12:42 UTC (permalink / raw) To: Jakub Jelinek Cc: Thomas Schwinge, Cesar Philippidis, Alexander Monakov, GCC Patches Hi, On Tue, Dec 13, 2016 at 12:43:16PM +0100, Jakub Jelinek wrote: > On Tue, Dec 13, 2016 at 12:39:01PM +0100, Thomas Schwinge wrote: > > On Fri, 9 Dec 2016 14:08:21 +0100, Martin Jambor <mjambor@suse.cz> wrote: > > > this is the promised attempt at splitting omp-low.c [...] > > > > Yay! \o/ > > > > I have not yet had a chance to review/test this patch, but I plan to. > > > > A few initial comments from the "bike shed departement"; I understand in > > GCC sources it will not be easy to rename stuff (such as files) later, so > > we should get the names agreed upon early: > > > > Generally, I agree with your division of "omp-low.c" parts. > > > > > - move everything that is part of pass_oacc_device_lower, > > > pass_omp_device_lower and pass_omp_target_link to a new file > > > omp-device.h, > > > > Should we call this file "omp-offload.c", as offloading is what this > > deals with, is the term we agreed to generally use (as far as I can > > tell)? > > That would be fine with me too. OK, will do. > > > > - move all pre-lowering gridification stuff to a new file > > > omp-grid.c. [...] > > > > Is that code generic enough to not call this file "omp-hsa.c" or similar? > Not at the moment, but... > And this as well. But omp-grid.c is fine too. ...I prefer omp-grid.c because I plan to use gridification also for GCN targets, though hopefully only as an optimization rather than a hard requirement ...and in fact I still think it is a good optimization of simple loops for execution on all CUDA-like environments with block/thread grids because it removes conditions which the run-time can handle better. > > > > - I moved stuff that was used from all over the place to a new file > > > omp-general.c (unless it would mean exposing omp_region or > > > omp_context types). > > > > I'd have called that simply "omp.c". > > The problem with that is that the corresponding header can't be called > omp.h for obvious reasons, we already have one with very different meaning. > That is exactly the reason why I chose omp-general. Thanks, Martin ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-13 12:42 ` Martin Jambor @ 2016-12-13 14:48 ` Cesar Philippidis 2016-12-14 13:16 ` Thomas Schwinge 1 sibling, 0 replies; 18+ messages in thread From: Cesar Philippidis @ 2016-12-13 14:48 UTC (permalink / raw) To: Jakub Jelinek, Thomas Schwinge, Alexander Monakov, GCC Patches, mjambor On 12/13/2016 04:42 AM, Martin Jambor wrote: >> And this as well. But omp-grid.c is fine too. > > ...I prefer omp-grid.c because I plan to use gridification also for > GCN targets, though hopefully only as an optimization rather than a > hard requirement ...and in fact I still think it is a good > optimization of simple loops for execution on all CUDA-like > environments with block/thread grids because it removes conditions > which the run-time can handle better. Regarding gridification, is your cauldron talk from 2015 still current, or have there been some significant changes? When we first started with OpenACC we were using a lot of the existing lower_omp_for* infrastructure handle ACC LOOPs. But there was a couple of problems with that. First, the chunk partitioning caused a lot of overhead, and second because of OpenACC execution model it made more sense to write our own functions (lower_oacc_head_tail / lower_oacc_reductions). In fact, during lowering gcc only marks where the loops are. All of those markers get replaced and the loops get optimized during the oaccdevlow pass which runs on the target compiler. Right now one of the significant bottlenecks we're experiencing on nvptx targets is with I/O. First, prior to launching a PTX kernel, libgomp transfers each data mapping individually in a synchronous manner. I'm debating whether it makes sense to pass in all of those data mappings to the accelerator prior to the PTX kernel launch asynchronously, obviously with an explicit synchronization barrier just prior to launching the kernel. Another bottleneck involves firstprivate variables. Often, those variables are 'scalars' and consequently, they shouldn't need explicit data mappings. I noticed that Jakub introduced a special GOMP_MAP_FIRSTPRIVATE_INT, which omits data mappings for integral types with less than or equal precision to pointers. It would probably be beneficial to expand this to reals. The last observation is that OpenMP code in general passes a struct with all of the data mappings to the various OMP regions/offloaded code. That's fine, but for offloading targets, particularly nvptx, it would probably be slightly more efficient if those OMP regions took actual function arguments instead of a single struct. At least on nvptx targets, in order to pass that struct to the accelerator, the runtime must first allocate device memory for it, then copy all of the struct contents to the device each time prior to launching a PTX kernel. A lot of this could be bypassed because cuLaunchKernel accepts a variable number of kernel arguments. Obviously, those arguments need to be transferred to the accelerator one way or another, so I'm not sure yet how beneficial this optimization would end up being. To be clear, I'm not proposing any of these changes for gcc7. Any changes to the above will go to gomp-4_0-branch first, then we'll port them over to gcc8. What type of performance problems are you experiencing with HSA? Cesar ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-13 12:42 ` Martin Jambor 2016-12-13 14:48 ` Cesar Philippidis @ 2016-12-14 13:16 ` Thomas Schwinge 2016-12-14 13:24 ` Martin Jambor 1 sibling, 1 reply; 18+ messages in thread From: Thomas Schwinge @ 2016-12-14 13:16 UTC (permalink / raw) To: Martin Jambor, Jakub Jelinek Cc: Cesar Philippidis, Alexander Monakov, GCC Patches Hi! On Tue, 13 Dec 2016 13:42:23 +0100, Martin Jambor <mjambor@suse.cz> wrote: > On Tue, Dec 13, 2016 at 12:43:16PM +0100, Jakub Jelinek wrote: > > On Tue, Dec 13, 2016 at 12:39:01PM +0100, Thomas Schwinge wrote: > > > On Fri, 9 Dec 2016 14:08:21 +0100, Martin Jambor <mjambor@suse.cz> wrote: > > > > this is the promised attempt at splitting omp-low.c [...] > > > > - move all pre-lowering gridification stuff to a new file > > > > omp-grid.c. [...] > > > > > > Is that code generic enough to not call this file "omp-hsa.c" or similar? > > > > Not at the moment, but... > > > And this as well. But omp-grid.c is fine too. > > ...I prefer omp-grid.c because I plan to use gridification also for > GCN targets, though hopefully only as an optimization rather than a > hard requirement ...and in fact I still think it is a good > optimization of simple loops for execution on all CUDA-like > environments with block/thread grids because it removes conditions > which the run-time can handle better. That certainly is reason enough to go with the generic name -- thanks for the explanation! > > > > - I moved stuff that was used from all over the place to a new file > > > > omp-general.c (unless it would mean exposing omp_region or > > > > omp_context types). > > > > > > I'd have called that simply "omp.c". > > > > The problem with that is that the corresponding header can't be called > > omp.h for obvious reasons, we already have one with very different meaning. > > That is exactly the reason why I chose omp-general. OK, that wasn't obvious to me, but yeah, it's probably best to avoid the name clash with libgomp's omp.h. I still couldn't allocate time to review the patch, but at least I now have tested it -- no regressions. As I suppose you want to commit this as sooner than later ;-) and you already have approval as I understand it, how about you do commit it, and I'll then later follow up, with any changes I'd additionally like to get done. Grüße Thomas ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-14 13:16 ` Thomas Schwinge @ 2016-12-14 13:24 ` Martin Jambor 0 siblings, 0 replies; 18+ messages in thread From: Martin Jambor @ 2016-12-14 13:24 UTC (permalink / raw) To: Thomas Schwinge Cc: Jakub Jelinek, Cesar Philippidis, Alexander Monakov, GCC Patches Hi, On Wed, Dec 14, 2016 at 02:14:32PM +0100, Thomas Schwinge wrote: > > I still couldn't allocate time to review the patch, but at least I now > have tested it -- no regressions. Great, thanks! > As I suppose you want to commit this > as sooner than later ;-) and you already have approval as I understand > it, how about you do commit it, and I'll then later follow up, with any > changes I'd additionally like to get done. > I am about to re-base, re-test, add the few formatting fixes that Jakub suggested to the second patch and commit. Hopefully all of it today. Martin ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2016-12-09 13:08 [PATCH] omp-low.c split Martin Jambor 2016-12-09 13:25 ` Alexander Monakov 2016-12-13 11:39 ` Thomas Schwinge @ 2021-08-04 12:40 ` Thomas Schwinge 2021-08-04 12:45 ` Jakub Jelinek 2 siblings, 1 reply; 18+ messages in thread From: Thomas Schwinge @ 2021-08-04 12:40 UTC (permalink / raw) To: Martin Jambor, gcc-patches; +Cc: Jakub Jelinek [-- Attachment #1: Type: text/plain, Size: 1291 bytes --] Hi! On 2016-12-09T14:08:21+0100, Martin Jambor <mjambor@suse.cz> wrote: > this is the promised attempt at splitting omp-low.c [...] > --- a/gcc/Makefile.in > +++ b/gcc/Makefile.in > @@ -2479,8 +2483,10 @@ GTFILES = $(CPP_ID_DATA_H) $(srcdir)/input.h $(srcdir)/coretypes.h \ > $(srcdir)/tree-scalar-evolution.c \ > $(srcdir)/tree-ssa-operands.h \ > $(srcdir)/tree-profile.c $(srcdir)/tree-nested.c \ > + $(srcdir)/omp-device.h \ > + $(srcdir)/omp-device.c \ > + $(srcdir)/omp-expand.c \ > $(srcdir)/omp-low.c \ > - $(srcdir)/omp-low.h \ > $(srcdir)/targhooks.c $(out_file) $(srcdir)/passes.c $(srcdir)/cgraphunit.c \ > $(srcdir)/cgraphclones.c \ > $(srcdir)/tree-phinodes.c \ 'gcc/omp-device.*' eventually got renamed to 'gcc/omp-offload.*'. OK to push the attached "Remove 'gcc/omp-offload.c' from 'GTFILES'"? | Given that it doesn't contain any 'GTY' markers, no 'gcc/gt-omp-offload.h' file | gets generated (and '#include'd anywhere). Grüße Thomas ----------------- Siemens Electronic Design Automation GmbH; Anschrift: Arnulfstraße 201, 80634 München; Gesellschaft mit beschränkter Haftung; Geschäftsführer: Thomas Heurung, Frank Thürauf; Sitz der Gesellschaft: München; Registergericht München, HRB 106955 [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #2: 0001-Remove-gcc-omp-offload.c-from-GTFILES.patch --] [-- Type: text/x-diff, Size: 1068 bytes --] From 1af32cf74a008a48328e82a6730b984f602b9979 Mon Sep 17 00:00:00 2001 From: Thomas Schwinge <thomas@codesourcery.com> Date: Wed, 4 Aug 2021 13:41:22 +0200 Subject: [PATCH] Remove 'gcc/omp-offload.c' from 'GTFILES' Given that it doesn't contain any 'GTY' markers, no 'gcc/gt-omp-offload.h' file gets generated (and '#include'd anywhere). Small fix-up for r243673 (Git commit 629b3d75c8c5a244d891a9c292bca6912d4b0dd9) "Split omp-low into multiple files". gcc/ * Makefile.in (GTFILES): Remove '$(srcdir)/omp-offload.c'. --- gcc/Makefile.in | 1 - 1 file changed, 1 deletion(-) diff --git a/gcc/Makefile.in b/gcc/Makefile.in index a9c9b506034..a3d9ee797df 100644 --- a/gcc/Makefile.in +++ b/gcc/Makefile.in @@ -2693,7 +2693,6 @@ GTFILES = $(CPPLIB_H) $(srcdir)/input.h $(srcdir)/coretypes.h \ $(srcdir)/tree-ssa-operands.h \ $(srcdir)/tree-profile.c $(srcdir)/tree-nested.c \ $(srcdir)/omp-offload.h \ - $(srcdir)/omp-offload.c \ $(srcdir)/omp-general.c \ $(srcdir)/omp-low.c \ $(srcdir)/targhooks.c $(out_file) $(srcdir)/passes.c \ -- 2.30.2 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: [PATCH] omp-low.c split 2021-08-04 12:40 ` Thomas Schwinge @ 2021-08-04 12:45 ` Jakub Jelinek 0 siblings, 0 replies; 18+ messages in thread From: Jakub Jelinek @ 2021-08-04 12:45 UTC (permalink / raw) To: Thomas Schwinge; +Cc: Martin Jambor, gcc-patches On Wed, Aug 04, 2021 at 02:40:27PM +0200, Thomas Schwinge wrote: > Small fix-up for r243673 (Git commit 629b3d75c8c5a244d891a9c292bca6912d4b0dd9) > "Split omp-low into multiple files". > > gcc/ > * Makefile.in (GTFILES): Remove '$(srcdir)/omp-offload.c'. Ok, thanks. > --- > gcc/Makefile.in | 1 - > 1 file changed, 1 deletion(-) > > diff --git a/gcc/Makefile.in b/gcc/Makefile.in > index a9c9b506034..a3d9ee797df 100644 > --- a/gcc/Makefile.in > +++ b/gcc/Makefile.in > @@ -2693,7 +2693,6 @@ GTFILES = $(CPPLIB_H) $(srcdir)/input.h $(srcdir)/coretypes.h \ > $(srcdir)/tree-ssa-operands.h \ > $(srcdir)/tree-profile.c $(srcdir)/tree-nested.c \ > $(srcdir)/omp-offload.h \ > - $(srcdir)/omp-offload.c \ > $(srcdir)/omp-general.c \ > $(srcdir)/omp-low.c \ > $(srcdir)/targhooks.c $(out_file) $(srcdir)/passes.c \ > -- > 2.30.2 > Jakub ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2021-08-04 12:45 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-12-09 13:08 [PATCH] omp-low.c split Martin Jambor 2016-12-09 13:25 ` Alexander Monakov 2016-12-09 13:53 ` Martin Jambor 2016-12-09 14:22 ` Jakub Jelinek 2016-12-09 16:19 ` Alexander Monakov 2016-12-13 10:20 ` Martin Jambor 2016-12-13 10:16 ` Martin Jambor 2016-12-13 10:35 ` Jakub Jelinek 2016-12-14 16:56 ` Martin Jambor 2016-12-13 12:58 ` Alexander Monakov 2016-12-13 11:39 ` Thomas Schwinge 2016-12-13 11:43 ` Jakub Jelinek 2016-12-13 12:42 ` Martin Jambor 2016-12-13 14:48 ` Cesar Philippidis 2016-12-14 13:16 ` Thomas Schwinge 2016-12-14 13:24 ` Martin Jambor 2021-08-04 12:40 ` Thomas Schwinge 2021-08-04 12:45 ` Jakub Jelinek
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).