On Sat, Nov 10, 2018 at 09:11:19AM -0800, Julian Brown wrote:
> This patch, created while trying to figure out the open-coded linked-list
> handling in gimplify_scan_omp_clauses, factors out four somewhat
> repetitive portions of that function into two new outlined functions.
> This was done largely mechanically; the actual lines of executed code are
> more-or-less the same. That means the interfaces to the new functions
> is somewhat eccentric though, and could no doubt be improved. I've tried
> to add commentary to the best of my understanding, but suggestions for
> improvements are welcome!
>
> As a bonus, one apparent bug introduced during an earlier refactoring
> to use the polynomial types has been fixed (I think!): "known_eq (o1,
> 2)" should have been "known_eq (o1, o2)".
>
> Tested alongside other patches in this series and the async patches. OK?
>
> ChangeLog
>
> gcc/
> * gimplify.c (insert_struct_component_mapping)
> (check_base_and_compare_lt): New.
I think
* gimplify.c (insert_struct_component_mapping,
check_base_and_compare_lt): New.
is what is used far more often than the above syntax.
> +
> +static tree
> +insert_struct_component_mapping (enum tree_code code, tree c, tree
> struct_node,
> + tree prev_node, tree *scp)
Please use a shorter name, like insert_struct_comp_mapping or even
insert_struct_comp_map, to avoid formatting glitches.
> +{
> + enum gomp_map_kind mkind = (code == OMP_TARGET_EXIT_DATA
> + || code == OACC_EXIT_DATA)
> + ? GOMP_MAP_RELEASE : GOMP_MAP_ALLOC;
Please use
enum gomp_map_kind mkind
= ((code == OMP_TARGET_EXIT_DATA || code == OACC_EXIT_DATA)
? GOMP_MAP_RELEASE : GOMP_MAP_ALLOC);
instead.
> + int base_eq_orig_base
> + = check_base_and_compare_lt (OMP_CLAUSE_DECL (c),
> + &orig_base, decl, &bitpos1, &offset1);
Incorrect formatting, &orig_base needs to be below OMP_CLAUSE_DECL. So:
int base_eq_orig_base
= check_base_and_compare_lt (OMP_CLAUSE_DECL (c),
&orig_base, decl, &bitpos1,
&offset1);
> + int same_decl_offset_lt
> + = check_base_and_compare_lt (
> + OMP_CLAUSE_DECL (*sc), NULL, decl,
> + &bitpos1, &offset1);
> + if (same_decl_offset_lt == -1)
Again, wrong formatting. If even the first argument doesn't fit, just use
a temporary.
tree sc_decl = OMP_CLAUSE_DECL (*sc);
int same_decl_offset_lt
= check_base_and_compare_lt (sc_decl, NULL, decl,
&bitpos1, &offset1);
> + tree cl
> + = insert_struct_component_mapping (code, c, NULL,
> + *prev_list_p, scp);
Also wrong formatting, should be:
tree cl
= insert_struct_component_mapping (code, c, NULL,
*prev_list_p,
scp);
or if the name is shorter, you can fit more.
> if (sc == prev_list_p)
> {
> *sc = cl;
Otherwise LGTM, but I admit I haven't verified every single statement.
Jakub
