Re: Validation of adding left shift stmt at GIMPLE - [Custom GCC plugin]]

[Shubham Narlawar <gsocshubham@gmail.com>]

On Wed, Feb 23, 2022 at 12:52 PM Richard Biener
<richard.guenther@gmail.com> wrote:
>
> On Tue, Feb 22, 2022 at 2:10 PM Shubham Narlawar <gsocshubham@gmail.com> wrote:
> >
> > On Tue, Feb 22, 2022 at 3:55 PM Richard Biener
> > <richard.guenther@gmail.com> wrote:
> > >
> > > On Tue, Feb 22, 2022 at 8:38 AM Shubham Narlawar <gsocshubham@gmail.com> wrote:
> > > >
> > > > On Mon, Feb 21, 2022 at 1:02 PM Richard Biener
> > > > <richard.guenther@gmail.com> wrote:
> > > > >
> > > > > On Sun, Feb 20, 2022 at 11:44 PM Andrew Pinski via Gcc <gcc@gcc.gnu.org> wrote:
> > > > > >
> > > > > > On Sun, Feb 20, 2022 at 10:45 AM Shubham Narlawar <gsocshubham@gmail.com> wrote:
> > > > > > >
> > > > > > > On Sat, Feb 19, 2022 at 1:15 AM Andrew Pinski <pinskia@gmail.com> wrote:
> > > > > > > >
> > > > > > > > On Fri, Feb 18, 2022 at 11:04 AM Shubham Narlawar via Gcc
> > > > > > > > <gcc@gcc.gnu.org> wrote:
> > > > > > > > >
> > > > > > > > > Hello,
> > > > > > > > >
> > > > > > > > > I want to know whether it is correct to add left shift instruction to
> > > > > > > > > a constant expression statement like "_3 + 4"?
> > > > > > > > >
> > > > > > > > > I am trying to add a left shift instruction in between below GIMPLE
> > > > > > > > > instructions -
> > > > > > > > >
> > > > > > > > >   <bb 2> :
> > > > > > > > >   instrn_buffer.0_1 = instrn_buffer;
> > > > > > > > >   _2 = tree.cnt;
> > > > > > > > >   _3 = (int) _2;
> > > > > > > > >   _4 = _3 + 4;
> > > > > > > > >   _5 = (unsigned int) _4;        // I want to add left shift here
> > > > > > > > >   D.2993 = __builtin_riscv_sfploadi (instrn_buffer.0_1, 0, _5);
> > > > > > > > > //this is "stmt"
> > > > > > > > >
> > > > > > > > > I am using this snippet in custom gcc plugin -
> > > > > > > > >
> > > > > > > > >           tree lshift_tmp = make_temp_ssa_name (integer_type_node,
> > > > > > > > > NULL, "slli");
> > > > > > > >
> > > > > > > > A couple of things.
> > > > > > > > I Noticed you use integer_type_node here. Why not the type of what is
> > > > > > > > being replaced?
> > > > > > > > That is the main thing I see right now.
> > > > > > >
> > > > > > > I want to apply left shift to a constant expression with 8 which is an
> > > > > > > integer. Since I am not replacing a statement, I am creating new
> > > > > > > GIMPLE statement -
> > > > > > >
> > > > > > > tree shift_amt = build_int_cst (integer_type_node, 8);
> > > > > > >
> > > > > > > Here, I am not replacing any GIMPLE statement. Is this the correct way
> > > > > > > to do this?
> > > > > > >
> > > > > > > My goal is to left shift constant expression and update its usage as below -
> > > > > > >
> > > > > > >   _19 = (unsigned int) _18;
> > > > > > >   D.2996 = __builtin_riscv_sfploadi (lexer.5_16, 12, _19);
> > > > > > >
> > > > > > > into
> > > > > > >
> > > > > > >   _19 = (unsigned int) _18;
> > > > > > > temp = _19 << 8
> > > > > > >   D.2996 = __builtin_riscv_sfploadi (lexer.5_16, 12, temp);
> > > > > > >
> > > > > > > I am storing the left shift result to the new ssa variable name "temp"
> > > > > > > and updating sfploadi parameters as expected.
> > > > > > >
> > > > > > > On doing the above, dom_walker_eliminate is prohibiting me to do the
> > > > > > > above gimple transformation. Is the above transformation complete and
> > > > > > > correct?
> > > > > >
> > > > > > I think you misunderstood me. I was saying for a left shift gimple,
> > > > > > the result type and the first operand type must be compatible (signed
> > > > > > and unsigned types are not compatible). In the above case, you have:
> > > > > > integer_type_node = unsigned_int << integer_type_name .
> > > > > >
> > > > > > Does that make sense now?
> > > > >
> > > > > Btw, the error you see is still odd - please make sure to build GCC with
> > > > > checking enabled or run your tests with -fchecking.  For further help
> > > >
> > > > Sure.
> > > >
> > > > > it might be useful to post the patch you are testing to show where exactly
> > > > > you are hooking into to add this statement.
> > > >
> > > > My goal is to transform below IR -
> > > >
> > > >   _5 = (unsigned int) _4;
> > > >   D.2993 = __builtin_riscv_* (instrn_buffer.0_1, 0, _5);
> > > >
> > > > to target IR -
> > > >
> > > >   _5 = (unsigned int) _4;
> > > >   lshiftamt_27 = _5 << 8;
> > > >   D.2996 = __builtin_riscv_* (instrn_buffer.0_1, 0, lshiftamt_27);
> > > >
> > > > I have followed this approach to build a new GIMPLE left shift
> > > > instruction - https://github.com/gcc-mirror/gcc/blob/0435b978f95971e139882549f5a1765c50682216/gcc/ubsan.cc#L1257
> > > >
> > > > Here is the patch which I am using -
> > > >
> > > > ------------------------------------------Patch-------------------------------------------------------
> > > > unsigned int
> > > > pass_custom_lowering::execute (function *fun)
> > > > {
> > > >   /* Code for iterating over all statements of function to find
> > > > function call of form - __builtin*
> > > >
> > > >   where one of parameter is constant expression of type "7 +
> > > > expression" i.e. 7 + _8
> > > >
> > > >   <bb 2> :
> > > >   instrn_buffer.0_1 = instrn_buffer;
> > > >   _2 = tree.cnt;
> > > >   _3 = (int) _2;
> > > >   _4 = _3 + 4;
> > > >   _5 = (unsigned int) _4;        // I want to apply left shift to _5
> > > >   D.2993 = __builtin_riscv_* (instrn_buffer.0_1, 0, _5);
> > > >
> > > >   */
> > > >           gcall *stmt = dyn_cast<gcall *> (g);     //here g is
> > > > __builtin_riscv_* where one of parameter is "_3 + 4"
> > > >           tree parm = gimple_call_arg (stmt, 2);
> > > >
> > > >           unsigned int shift = 8;
> > > >           tree shift_amt = build_int_cst (unsigned_type_node, shift);
> > > >           tree lshift_tmp_name = make_temp_ssa_name
> > > > (unsigned_type_node, NULL, "slli");
> > > >           gimple *lshift_stmt = gimple_build_assign (lshift_tmp_name,
> > > > LSHIFT_EXPR, parm,
> > > >                               shift_amt);
> > > >           gsi_insert_before(&gsi, lshift_stmt, GSI_NEW_STMT);
> > > >           gimple_call_set_arg (stmt, 2, lshift_tmp_name);
> > > >           //update_stmt (stmt);
> > >
> > > This update_stmt is required
> >
> > Got it. Thanks!
> >
> > >
> > > >           //update_ssa (TODO_update_ssa);
> > > >
> > > >   return 0;
> > > > }
> > > > ---------------------------------------------------------------------------------------------------------
> > > >
> > > > Is the above code correct?
> > >
> > > Yes, the code is incomplete of course, it misses how you get to 'g'
> > > but I assume it's just FOR_EACH_BB_FN and iteration over each
> > > BBs stmts.
> >
> > Yes. These are present FOR_EACH_BB_FN in my code to identify specific
> > GIMPLE_CALL statements.
> >
> > After adding all above suggestions, fixup_cfg is failing due to the
> > above transformation. I am thinking that the position of the pass
> > might be the problem.
> >
> > Currently my pass "pass_custom_lowering" is present at end of
> > "all_lowering_pass as below -
> >
> >   INSERT_PASSES_AFTER (all_lowering_passes)
> >   NEXT_PASS (pass_warn_unused_result);
> >   NEXT_PASS (pass_diagnose_omp_blocks);
> >   NEXT_PASS (pass_diagnose_tm_blocks);
> > ..........
> >   NEXT_PASS (pass_build_cfg);
> > ........
> >   NEXT_PASS (pass_build_cgraph_edges);
> >   NEXT_PASS (pass_custom_lowering);
> >   TERMINATE_PASS_LIST (all_lowering_passes)
> >
> > Can you suggest a proper place for the pass for the above kind of
> > transformation?
>
> I don't see a particular problem with this place but I would suggest
> to move it into the pass_build_ssa_passes group, before pass_ubsan
> for example (but certainly after pass_build_ssa).  Given you run
> pre SSA build maybe you simply have stray TODO not appropriate
> in your pass_data.
>
> Anyway, try moving the pass after build-ssa.

Hello,

[1] With the above suggestion - I was able to do the desired
transformation using a plugin. I am generating custom left shift,
right shift, load immediate and an add instruction just before a
specific function call.

e.g.

  slli_37 = _2 << 8;
  srli_38 = slli_37 >> 8;
  li_39 = (unsigned int) 15;
  add_40 = srli_38 + li_39;
  _22 = __builtin_* (instrn_buffer.36_1, 1, add_40);


[2] My ultimate aim is to identify and load immediate instruction
"li_39 = (unsigned int) 15;" during cfgexpand pass and store rtx
instructions associated with it. Then in the custom rtl pass after
register allocation pass, I want to do some transformation on it. But
this instruction is optimized by ccp and hence I am losing the
placeholder.

----------I am generating gimple instruction "li" using----------------
              tree load = build_int_cst (unsigned_type_node, 15);
              tree li_tmp_name = make_temp_ssa_name
(unsigned_type_node, NULL, "li");
              gimple *li_stmt = gimple_build_assign (li_tmp_name,
NOP_EXPR, load);
              gsi_insert_after (&gsi, li_stmt, GSI_NEW_STMT);
---------------------------------------------------------------------

My aim is to prohibit the above but it seems incorrect to just disable
ccp. Please give any suggestions on retaining the statement.

1. To prohibit above, I tried to assign global variable to "li" as below -

              tree li_tmp_name = make_temp_ssa_name
(integer_type_node, NULL, "li");
              tree foo_tmp_name = build_translation_unit_decl
(create_tmp_var_name("foo"));
              DECL_EXTERNAL(foo_tmp_name) = 1;
              gimple *li_stmt = gimple_build_assign (li_tmp_name,
NOP_EXPR, foo_tmp_name);
              gsi_insert_after (&gsi, li_stmt, GSI_NEW_STMT);

which generates - (but it crashes)

li_51 = (int) <<< Unknown tree: translation_unit_decl >>>;

Can you please suggest to me the correct way to achieve the above?

Thanks and Regards,
Shubham

>
> > Thanks and Regards,
> > Shubham
> >
> > >
> > > > I was hoping to do the below transformation using above code but feel
> > > > there is some missing operation that needs to be added to above code.
> > > > The goal is simple to generate a left shift to the 3rd parameter of
> > > > function which is constant expression.
> > > >
> > > >   _5 = (unsigned int) _4;
> > > >   D.2993 = __builtin_riscv_* (instrn_buffer.0_1, 0, _5);
> > > >
> > > > to
> > > >
> > > >   _5 = (unsigned int) _4;
> > > >   lshiftamt_27 = _5 << 8;
> > > >   D.2996 = __builtin_riscv_* (instrn_buffer.0_1, 0, lshiftamt_27);
> > > >
> > > > Thanks and Regards,
> > > > Shubham
> > > >
> > > >
> > > > >
> > > > > Richard.
> > > > >
> > > > > > Thanks,
> > > > > > Andrew
> > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Also you shouldn't need to do:
> > > > > > > > update_ssa (TODO_update_ssa);
> > > > > > > >
> > > > > > > > As make_temp_ssa_name is a new SSA name already and such.
> > > > > > >
> > > > > > > Understood.
> > > > > > >
> > > > > > > Thanks and Regards,
> > > > > > > Shubham
> > > > > > >
> > > > > > >
> > > > > > > >
> > > > > > > > Thanks,
> > > > > > > > Andrew Pinski
> > > > > > > >
> > > > > > > > >           gimple *lshift = gimple_build_assign (lshift_tmp, LSHIFT_EXPR, parm,
> > > > > > > > >                                                       build_int_cst
> > > > > > > > > (integer_type_node, 8));
> > > > > > > > >           gsi_insert_before(&gsi, lshift, GSI_NEW_STMT);
> > > > > > > > >           //Update function call
> > > > > > > > >           gimple_call_set_arg (stmt, idx, lshift_tmp);
> > > > > > > > >           update_stmt (stmt);
> > > > > > > > >           update_ssa (TODO_update_ssa);
> > > > > > > > >
> > > > > > > > > from which above GIMPLE IR is modified to -
> > > > > > > > >
> > > > > > > > >   <bb 2> :
> > > > > > > > >   instrn_buffer.0_1 = instrn_buffer;
> > > > > > > > >   _2 = tree.cnt;
> > > > > > > > >   _3 = (int) _2;
> > > > > > > > >   _4 = _3 + 4;
> > > > > > > > >   _5 = (unsigned int) _4;
> > > > > > > > >   slli_24 = _5 << 8;
> > > > > > > > >   D.2993 = __builtin_riscv_sfploadi (instrn_buffer.0_1, 0, slli_24);
> > > > > > > > >
> > > > > > > > >
> > > > > > > > > 1. When I run above code, either dominator tree validation or tree cfg
> > > > > > > > > fixup is failing which suggests to me it is either incorrect to apply
> > > > > > > > > such left shift or some more work is missing?
> > > > > > > > >
> > > > > > > > > 2. I followed how a left shift gimple assignment is generated but
> > > > > > > > > still feels there is something wrong with the above generation. Can
> > > > > > > > > someone please point me out?
> > > > > > > > >
> > > > > > > > > Thanks in advance! As always, the GCC community and its members are
> > > > > > > > > very supportive, responsive and helpful!
> > > > > > > > >
> > > > > > > > > Regards,
> > > > > > > > > Shubham