On Mon, Jan 29, 2024 at 9:34 AM H.J. Lu <[email protected]> wrote:
>
> On Mon, Jan 29, 2024 at 9:00 AM Jakub Jelinek <[email protected]> wrote:
> >
> > On Mon, Jan 29, 2024 at 08:45:45AM -0800, H.J. Lu wrote:
> > > In this case, these are internal to the same comdat group:
> >
> > But that is only by accident, no?
>
> This may be by luck. I don't know if gcc checks it when
> generating such references.
>
> > I mean, if you need to refer to such a symbol from
> > non-comdat function or comdat function in a different comdat group
> > and RA decides it wants the constant in memory rather than code?
> > Your patch uses
> > if (decl)
> > return targetm.asm_out.function_rodata_section (decl, ???);
> > and default_function_rodata_section only looks at comdat group of the
> > passed in decl. But the decl here is what the constant refers to, not
> > who is referring it.
LRA puts a function symbol reference in a constant pool via
#0 force_const_mem (in_mode=E_DImode, x=0x7fffe9e7e000)
at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/varasm.cc:3951
#1 0x0000000001833870 in curr_insn_transform (check_only_p=false)
at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/lra-constraints.cc:4473
#2 0x0000000001836eae in lra_constraints (first_p=true)
at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/lra-constraints.cc:5462
#3 0x000000000181fcf1 in lra (f=0x0, verbose=5)
at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/lra.cc:2442
#4 0x00000000017c8828 in do_reload ()
at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/ira.cc:5973
#5 0x00000000017c8d25 in (anonymous namespace)::pass_reload::execute (
this=0x48d8730)
at /export/gnu/import/git/gitlab/x86-gcc-test/gcc/ira.cc:6161
for
(gdb) call debug_rtx (curr_insn)
(insn 12 57 15 2 (set (reg:V2DI 101 [ _16 ])
(vec_concat:V2DI (symbol_ref:DI
("_ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE")
[flags 0x3] <function_decl 0x7fffe9e2a600 _M_manager>)
(reg/f:DI 109))) 7521 {vec_concatv2di}
(expr_list:REG_DEAD (reg/f:DI 110)
(expr_list:REG_DEAD (reg/f:DI 109)
(expr_list:REG_EQUIV (vec_concat:V2DI (symbol_ref:DI
("_ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE")
[flags 0x3] <function_decl 0x7fffe9e2a600 _M_manager>)
(symbol_ref:DI
("_ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE9_M_invokeERKNS_9_Any_dataE")
[flags 0x3] <function_decl 0x7fffe9e2a700 _M_invoke>))
(nil)))))
(gdb)
CONST_POOL_OK_P doesn't check if it is safe to do so for function
symbols. Here is a patch to add the check.
--
H.J.
From 1947920740e48cdc8076299f8cc58e797ec39a7c Mon Sep 17 00:00:00 2001
From: "H.J. Lu" <[email protected]>
Date: Mon, 29 Jan 2024 12:53:32 -0800
Subject: [PATCH] lra: Add const_pool_reference_ok
LRA may put a function symbol reference in
(gdb) call debug_rtx (curr_insn)
(insn 12 57 15 2 (set (reg:V2DI 101 [ _16 ])
(vec_concat:V2DI (symbol_ref:DI ("_ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE") [flags 0x3] <function_decl 0x7fffe9e2a600 _M_manager>)
(reg/f:DI 109))) 7521 {vec_concatv2di}
(expr_list:REG_DEAD (reg/f:DI 110)
(expr_list:REG_DEAD (reg/f:DI 109)
(expr_list:REG_EQUIV (vec_concat:V2DI (symbol_ref:DI ("_ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE10_M_managerERNS_9_Any_dataERKSI_NS_18_Manager_operationE") [flags 0x3] <function_decl 0x7fffe9e2a600 _M_manager>)
(symbol_ref:DI ("_ZN4blah17_Function_handlerIFvvENS_5_BindIFPFvPvxxxEPN3vtk6detail3smp27vtkSMPTools_FunctorInternalIN12_GLOBAL__N_19CountUsesIxEELb0EEExxxEEEE9_M_invokeERKNS_9_Any_dataE") [flags 0x3] <function_decl 0x7fffe9e2a700 _M_invoke>))
(nil)))))
(gdb)
in the constant pool. But it isn't safe when the referenced function
symbol is in a different COMDAT group from the current instruction
function body if the function symbol isn't public.
Add const_pool_reference_ok to check if a function symbol can be forced
into the constant pool.
PR rtl-optimization/113617
* lra-constraints.cc (const_pool_reference_ok): New.
(CONST_POOL_OK_P): Use.
---
gcc/lra-constraints.cc | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/gcc/lra-constraints.cc b/gcc/lra-constraints.cc
index 0ae81c1ff9c..59e6944c245 100644
--- a/gcc/lra-constraints.cc
+++ b/gcc/lra-constraints.cc
@@ -925,6 +925,35 @@ operands_match_p (rtx x, rtx y, int y_hard_regno)
return true;
}
+/* Return true if the symbol X can be referenced in the function
+ FUNC_DECL. */
+
+static bool
+const_pool_reference_ok (tree func_decl, rtx x)
+{
+ /* It is OK if there is no COMDAT or X isn't a symbol. */
+ if (!HAVE_COMDAT_GROUP || GET_CODE (x) != SYMBOL_REF)
+ return true;
+
+ tree decl = SYMBOL_REF_DECL (x);
+ /* It is OK if X isn't a function symbol. */
+ if (TREE_CODE (decl) != FUNCTION_DECL)
+ return true;
+
+ tree comdat_group = DECL_COMDAT_GROUP (decl);
+ /* It is OK if X isn't a COMDAT symbol. */
+ if (!comdat_group)
+ return true;
+
+ /* It is OK if X and FUNC_DECL are in the same COMDAT group. */
+ if (comdat_group == DECL_COMDAT_GROUP (func_decl))
+ return true;
+
+ /* X is referenced from a different COMDAT group. Return true only
+ if X is a public symbol. */
+ return TREE_PUBLIC (decl) != 0;
+}
+
/* True if X is a constant that can be forced into the constant pool.
MODE is the mode of the operand, or VOIDmode if not known. */
#define CONST_POOL_OK_P(MODE, X) \
@@ -932,6 +961,7 @@ operands_match_p (rtx x, rtx y, int y_hard_regno)
&& CONSTANT_P (X) \
&& GET_CODE (X) != HIGH \
&& GET_MODE_SIZE (MODE).is_constant () \
+ && const_pool_reference_ok (cfun->decl, X) \
&& !targetm.cannot_force_const_mem (MODE, X))
/* If REG is a reload pseudo, try to make its class satisfying CL. */
--
2.43.0
