On Sun, Sep 29, 2024 at 8:01 PM Jeff Law <jeffreya...@gmail.com> wrote: > > > > On 9/13/24 5:06 AM, Mariam Arutunian wrote: > > Symbolically execute potential CRC loops and check whether the loop > > actually calculates CRC (uses LFSR matching). > > Calculated CRC and created LFSR are compared on each iteration of the > > potential CRC loop. > > > > gcc/ > > > > * Makefile.in (OBJS): Add crc-verification.o. > > * crc-verification.cc: New file. > > * crc-verification.h: New file. > > * gimple-crc-optimization.cc (loop_calculates_crc): New function. > > (is_output_crc): Likewise. > > (swap_crc_and_data_if_needed): Likewise. > > (validate_crc_and_data): Likewise. > > (optimize_crc_loop): Likewise. > > (table_based_may_be_generated): Likewise. > > (get_output_phi): Likewise. > > (execute): Add check whether potential CRC loop calculates CRC. > > > > gcc/sym-exec/ > > > > * sym-exec-state.cc (create_reversed_lfsr): New function. > > (create_forward_lfsr): Likewise. > > (last_set_bit): Likewise. > > (create_lfsr): Likewise. > > * sym-exec-state.h (is_bit_vector): Reorder, make the function > > public and static. > > (create_reversed_lfsr): New static function declaration. > > (create_forward_lfsr): New static function declaration. > > > > Signed-off-by: Mariam Arutunian <mariamarutun...@gmail.com > > <mailto:mariamarutun...@gmail.com>> > > Mentored-by: Jeff Law <j...@ventanamicro.com <mailto:j...@ventanamicro.com>> > There's a bit of TARGET_XXX stuff in here too that needs to be removed > as I noted in the review of a prior patch in this series: > > > +/* Returns true if table-based CRC may be generated, > > + otherwise, return false. */ > > + > > +bool > > +crc_optimization::table_based_may_be_generated () > > +{ > > + #ifdef TARGET_CRC32 > > + if (TARGET_CRC32 > > + && (m_polynomial == 0x1EDC6F41 || m_polynomial == 0x04C11DB7)) > > + return false; > > + #endif > > + /* If CLMUL-based CRC can be generated, return false. */ > > + if (is_CLMUL_supported () > > + && (GET_MODE_SIZE (TYPE_MODE (TREE_TYPE (m_crc_arg))).to_constant () > > + < GET_MODE_SIZE (word_mode))) > > + return false; > > + return true; > > +} > > Removing this probably will result in minor simplifications elsewhere in > this file. > > > This is a bit puzzling: > > > +bool > > +assign_calc_vals_to_header_phis (const vec<state *> &prev_states, > > + state *curr_state, > > + basic_block bb) > > +{ > > + for (gphi_iterator gsi = gsi_start_phis (bb); !gsi_end_p (gsi); > > + gsi_next (&gsi)) > > + { > > + gphi *phi = gsi.phi (); > > + tree lhs = gimple_phi_result (phi); > > + > > + /* Don't consider virtual operands. */ > > + if (virtual_operand_p (lhs)) > > + continue; > > + > > + if (TREE_CODE (PHI_ARG_DEF (phi, phi->nargs - 1)) == INTEGER_CST) > > The order of elements in the PHI is undefined, it looks like this > assumes the constant is always last. So it probably needs to be > generalized a bit. > > Similarly in assign_known_vals_to_header_phis.
As in the other case I noted some time ago you generally want to use gimple_phi_arg_def_from_edge and use either the latch or the preheader edge to identify a specifc PHI argument. > Mostly looks OK. I suspect with the issues noted above fixed that this > will be good to go. > > > Jeff
