On Tue, Jul 30, 2024 at 7:33 PM Tobias Burnus <tbur...@baylibre.com> wrote:
>
> Richard Biener wrote:
> > On Mon, Jul 29, 2024 at 9:26 PM Tobias Burnus<tbur...@baylibre.com> wrote:
> >> Inside pass_omp_target_link::execute, there is a call to
> >> gimple_regimplify_operands but the value expression is not
> >> expanded.[...]
> >>
> >> Where is_gimple_mem_ref_addr is defined as:
> >>
> >> /* Return true if T is a valid address operand of a MEM_REF. */
> >>
> >> bool
> >> is_gimple_mem_ref_addr (tree t)
> >> {
> >> return (is_gimple_reg (t)
> >> || TREE_CODE (t) == INTEGER_CST
> >> || (TREE_CODE (t) == ADDR_EXPR
> >> && (CONSTANT_CLASS_P (TREE_OPERAND (t, 0))
> >> || decl_address_invariant_p (TREE_OPERAND (t, 0)))));
> >> }
> > I think iff then decl_address_invariant_p should be amended.
>
> This does not work - at least not for my use case if OpenMP
> link variables - due to ordering issues.
>
> For the device compilers, the VALUE_EXPR is added in lto_main
> or in do_whole_program_analysis (same file: lto/lto.cc) by
> callingoffload_handle_link_vars. The value expression is then later expanded
> via pass_omp_target_link::execute, but in between the following happens:
>
> lto_main callssymbol_table::compile, which then calls
> cgraph_node::expand and that executes
>
> res |= verify_types_in_gimple_reference (lhs, true); for lhs being:
> MEM <uint128_t> [(c_char * {ref-all})&arr2]
> But when adding the has-value-expr check either directly to
> is_gimple_mem_ref_addr or to the decl_address_invariant_pit calls, the
> following condition becomes true the called function in
> tree-cfg.cc:
>
> 3302 if (!is_gimple_mem_ref_addr (TREE_OPERAND (expr, 0))
> 3303 || (TREE_CODE (TREE_OPERAND (expr, 0)) == ADDR_EXPR
> 3304 && verify_address (TREE_OPERAND (expr, 0), false)))
> 3305 {
> 3306 error ("invalid address operand in %qs", code_name);
>
> * * * Thus, I am now back to the previous change, except for:
>
> > Why is the gimplify_addr_expr hunk needed? It should get
> > to gimplifying the VAR_DECL/PARM_DECL by recursion?
>
> Indeed. I wonder why I had (thought to) need it before; possibly
> because it was needed or thought to be needed when trying to trace
> this down.
>
> Previous patch - except for that bit removed - attached.
>
> Thoughts, better ideas?
Looking at pass_omp_target_link::execute I wonder iff find_link_var_op
shouldn't simply do the substitution? Aka
diff --git a/gcc/omp-offload.cc b/gcc/omp-offload.cc
index 35313c2ecf3..cf9e5b715ab 100644
--- a/gcc/omp-offload.cc
+++ b/gcc/omp-offload.cc
@@ -2893,6 +2893,7 @@ find_link_var_op (tree *tp, int *walk_subtrees, void *)
&& is_global_var (t)
&& lookup_attribute ("omp declare target link", DECL_ATTRIBUTES (t)))
{
+ *tp = unshare_expr (DECL_VALUE_EXPR (t));
*walk_subtrees = 0;
return t;
}
which then makes the stmt obviously not gimple?
>
> Tobias
