| Message ID | 20230623144847.85698-1-juzhe.zhong@rivai.ai |
|---|---|
| State | New |
| Headers | show |
| Series | DSE: Add LEN_MASK_STORE analysis into DSE | expand |
On 6/23/23 08:48, juzhe.zhong@rivai.ai wrote: > From: Ju-Zhe Zhong <juzhe.zhong@rivai.ai> > > gcc/ChangeLog: > > * tree-ssa-dse.cc (initialize_ao_ref_for_dse): Add LEN_MASK_STORE. > (dse_optimize_stmt): Ditto. > > --- > gcc/tree-ssa-dse.cc | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc > index 3c7a2e9992d..01b0951f1a9 100644 > --- a/gcc/tree-ssa-dse.cc > +++ b/gcc/tree-ssa-dse.cc > @@ -174,6 +174,23 @@ initialize_ao_ref_for_dse (gimple *stmt, ao_ref *write, bool may_def_ok = false) > return true; > } > break; > + case IFN_LEN_MASK_STORE: { > + /* We cannot initialize a must-def ao_ref (in all cases) but we > + can provide a may-def variant. */ > + if (may_def_ok) > + { > + tree len_size > + = int_const_binop (MINUS_EXPR, gimple_call_arg (stmt, 2), > + gimple_call_arg (stmt, 5)); > + tree mask_size > + = TYPE_SIZE_UNIT (TREE_TYPE (gimple_call_arg (stmt, 4))); > + tree size = int_const_binop (MAX_EXPR, len_size, mask_size); > + ao_ref_init_from_ptr_and_size (write, gimple_call_arg (stmt, 0), > + size); So isn't len_size here the size in elements? If so, don't you need to multiply len_size by the element size? Jeff
Address comment. juzhe.zhong@rivai.ai From: Jeff Law Date: 2023-06-23 22:57 To: juzhe.zhong; gcc-patches CC: rguenther; richard.sandiford Subject: Re: [PATCH] DSE: Add LEN_MASK_STORE analysis into DSE On 6/23/23 08:48, juzhe.zhong@rivai.ai wrote: > From: Ju-Zhe Zhong <juzhe.zhong@rivai.ai> > > gcc/ChangeLog: > > * tree-ssa-dse.cc (initialize_ao_ref_for_dse): Add LEN_MASK_STORE. > (dse_optimize_stmt): Ditto. > > --- > gcc/tree-ssa-dse.cc | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc > index 3c7a2e9992d..01b0951f1a9 100644 > --- a/gcc/tree-ssa-dse.cc > +++ b/gcc/tree-ssa-dse.cc > @@ -174,6 +174,23 @@ initialize_ao_ref_for_dse (gimple *stmt, ao_ref *write, bool may_def_ok = false) > return true; > } > break; > + case IFN_LEN_MASK_STORE: { > + /* We cannot initialize a must-def ao_ref (in all cases) but we > + can provide a may-def variant. */ > + if (may_def_ok) > + { > + tree len_size > + = int_const_binop (MINUS_EXPR, gimple_call_arg (stmt, 2), > + gimple_call_arg (stmt, 5)); > + tree mask_size > + = TYPE_SIZE_UNIT (TREE_TYPE (gimple_call_arg (stmt, 4))); > + tree size = int_const_binop (MAX_EXPR, len_size, mask_size); > + ao_ref_init_from_ptr_and_size (write, gimple_call_arg (stmt, 0), > + size); So isn't len_size here the size in elements? If so, don't you need to multiply len_size by the element size? Jeff
On Fri, 23 Jun 2023, juzhe.zhong@rivai.ai wrote: > From: Ju-Zhe Zhong <juzhe.zhong@rivai.ai> > > gcc/ChangeLog: > > * tree-ssa-dse.cc (initialize_ao_ref_for_dse): Add LEN_MASK_STORE. > (dse_optimize_stmt): Ditto. > > --- > gcc/tree-ssa-dse.cc | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc > index 3c7a2e9992d..01b0951f1a9 100644 > --- a/gcc/tree-ssa-dse.cc > +++ b/gcc/tree-ssa-dse.cc > @@ -174,6 +174,23 @@ initialize_ao_ref_for_dse (gimple *stmt, ao_ref *write, bool may_def_ok = false) > return true; > } > break; > + case IFN_LEN_MASK_STORE: { > + /* We cannot initialize a must-def ao_ref (in all cases) but we > + can provide a may-def variant. */ > + if (may_def_ok) > + { > + tree len_size > + = int_const_binop (MINUS_EXPR, gimple_call_arg (stmt, 2), > + gimple_call_arg (stmt, 5)); please use the accessors for the mask/len operand number where available. Also I think this shows the existing IFN_LEN_STORE support is bogus since the LEN argument isn't guaranteed to be constant. Can you fix this as well please? If the length isn't constant the code should be the same as the IFN_MASK_STORE variant. > + tree mask_size > + = TYPE_SIZE_UNIT (TREE_TYPE (gimple_call_arg (stmt, 4))); > + tree size = int_const_binop (MAX_EXPR, len_size, mask_size); > + ao_ref_init_from_ptr_and_size (write, gimple_call_arg (stmt, 0), > + size); > + return true; > + } > + break; > + } > default:; > } > } > @@ -1502,6 +1519,7 @@ dse_optimize_stmt (function *fun, gimple_stmt_iterator *gsi, sbitmap live_bytes) > { > case IFN_LEN_STORE: > case IFN_MASK_STORE: > + case IFN_LEN_MASK_STORE: > { > enum dse_store_status store_status; > store_status = dse_classify_store (&ref, stmt, false, live_bytes); >
Hi, Richi. >> please use the accessors for the mask/len operand number where available. >>Also I think this shows the existing IFN_LEN_STORE support is bogus >>since the LEN argument isn't guaranteed to be constant. Can you >>fix this as well please? If the length isn't constant the code should >>be the same as the IFN_MASK_STORE variant. I understand your idea. Forget about V2 patch (which I sent). I will send V3 patch to fix both LEN_STORE and LEN_MASK_STORE. Thank you so much for the review! juzhe.zhong@rivai.ai From: Richard Biener Date: 2023-06-26 14:28 To: Ju-Zhe Zhong CC: gcc-patches; richard.sandiford Subject: Re: [PATCH] DSE: Add LEN_MASK_STORE analysis into DSE On Fri, 23 Jun 2023, juzhe.zhong@rivai.ai wrote: > From: Ju-Zhe Zhong <juzhe.zhong@rivai.ai> > > gcc/ChangeLog: > > * tree-ssa-dse.cc (initialize_ao_ref_for_dse): Add LEN_MASK_STORE. > (dse_optimize_stmt): Ditto. > > --- > gcc/tree-ssa-dse.cc | 18 ++++++++++++++++++ > 1 file changed, 18 insertions(+) > > diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc > index 3c7a2e9992d..01b0951f1a9 100644 > --- a/gcc/tree-ssa-dse.cc > +++ b/gcc/tree-ssa-dse.cc > @@ -174,6 +174,23 @@ initialize_ao_ref_for_dse (gimple *stmt, ao_ref *write, bool may_def_ok = false) > return true; > } > break; > + case IFN_LEN_MASK_STORE: { > + /* We cannot initialize a must-def ao_ref (in all cases) but we > + can provide a may-def variant. */ > + if (may_def_ok) > + { > + tree len_size > + = int_const_binop (MINUS_EXPR, gimple_call_arg (stmt, 2), > + gimple_call_arg (stmt, 5)); please use the accessors for the mask/len operand number where available. Also I think this shows the existing IFN_LEN_STORE support is bogus since the LEN argument isn't guaranteed to be constant. Can you fix this as well please? If the length isn't constant the code should be the same as the IFN_MASK_STORE variant. > + tree mask_size > + = TYPE_SIZE_UNIT (TREE_TYPE (gimple_call_arg (stmt, 4))); > + tree size = int_const_binop (MAX_EXPR, len_size, mask_size); > + ao_ref_init_from_ptr_and_size (write, gimple_call_arg (stmt, 0), > + size); > + return true; > + } > + break; > + } > default:; > } > } > @@ -1502,6 +1519,7 @@ dse_optimize_stmt (function *fun, gimple_stmt_iterator *gsi, sbitmap live_bytes) > { > case IFN_LEN_STORE: > case IFN_MASK_STORE: > + case IFN_LEN_MASK_STORE: > { > enum dse_store_status store_status; > store_status = dse_classify_store (&ref, stmt, false, live_bytes); >
diff --git a/gcc/tree-ssa-dse.cc b/gcc/tree-ssa-dse.cc index 3c7a2e9992d..01b0951f1a9 100644 --- a/gcc/tree-ssa-dse.cc +++ b/gcc/tree-ssa-dse.cc @@ -174,6 +174,23 @@ initialize_ao_ref_for_dse (gimple *stmt, ao_ref *write, bool may_def_ok = false) return true; } break; + case IFN_LEN_MASK_STORE: { + /* We cannot initialize a must-def ao_ref (in all cases) but we + can provide a may-def variant. */ + if (may_def_ok) + { + tree len_size + = int_const_binop (MINUS_EXPR, gimple_call_arg (stmt, 2), + gimple_call_arg (stmt, 5)); + tree mask_size + = TYPE_SIZE_UNIT (TREE_TYPE (gimple_call_arg (stmt, 4))); + tree size = int_const_binop (MAX_EXPR, len_size, mask_size); + ao_ref_init_from_ptr_and_size (write, gimple_call_arg (stmt, 0), + size); + return true; + } + break; + } default:; } } @@ -1502,6 +1519,7 @@ dse_optimize_stmt (function *fun, gimple_stmt_iterator *gsi, sbitmap live_bytes) { case IFN_LEN_STORE: case IFN_MASK_STORE: + case IFN_LEN_MASK_STORE: { enum dse_store_status store_status; store_status = dse_classify_store (&ref, stmt, false, live_bytes);
From: Ju-Zhe Zhong <juzhe.zhong@rivai.ai> gcc/ChangeLog: * tree-ssa-dse.cc (initialize_ao_ref_for_dse): Add LEN_MASK_STORE. (dse_optimize_stmt): Ditto. --- gcc/tree-ssa-dse.cc | 18 ++++++++++++++++++ 1 file changed, 18 insertions(+)