This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH, Pointer Bounds Checker 14/x] Passes [12/n] Optimize string functions


On 10/08/14 13:18, Ilya Enkovich wrote:
Hi,

This patch introduces simple optimization of string function calls using variants with no checks and/or bounds copy when possible.

Thanks,
Ilya
--
2014-10-08  Ilya Enkovich  <ilya.enkovich@intel.com>

	* tree-chkp.c (check_infos): New.
	(chkp_get_nobnd_fndecl): New.
	(chkp_get_nochk_fndecl): New.
	(chkp_optimize_string_function_calls): New.
	(chkp_opt_init): New.
	(chkp_opt_fini): New.
	(chkp_opt_execute): New.
	(chkp_opt_gate): New.
	(pass_data_chkp_opt): New.
	(pass_chkp_opt): New.
	(make_pass_chkp_opt): New.
Just a note, these reviews are going to come out-of-order.

I thought I mentioned it in the prior message. Can you pull the optimization stuff into its own file. It seems to me it ought to be conceptually separate from expansion and the other miscellaneous stuff in tree-chkp.c.


+
+/* Find memcpy, mempcpy, memmove and memset calls, perform
+   checks before call and then call no_chk version of
+   functions.  We do it on O2 to enable inlining of these
+   functions during expand.
So is the purpose here to expose the checks that would normally be done in the mem* routines to their caller in the hopes that doing so will expose redundant checks? Or is there some other reason?


+
+   Also try to find memcpy, mempcpy, memmove and memset calls
+   which are known to not write pointers to memory and use
+   faster function versions for them.  */
Can you please include tests for both classes of transformations? It doesn't have to be exhaustive, just a few simple tests to both verify the transformation is working today and to help ensure nobody breaks it in the future?


+void
+chkp_optimize_string_function_calls (void)
+{
+  basic_block bb;
+
+  if (dump_file && (dump_flags & TDF_DETAILS))
+    fprintf (dump_file, "Searching for replacable string function calls...\n");
s/replacable/replaceable/

Seems like if you're going to emit a message into the debug file that the compiler is searching for replaceable calls that you ought to emit a message when a replaceable call is found too.

And doing so ought to make writing those testcases easier too since you can scan the appropriate dump file for the message :-)


+
+	      if (fndecl_nochk)
+		fndecl = fndecl_nochk;
+
+	      gimple_call_set_fndecl (stmt, fndecl);
+
+	      /* If there is no nochk version of function then
+		 do nothing.  Otherwise insert checks before
+		 the call.  */
+	      if (!fndecl_nochk)
+		continue;
It's a nit, but I'd tend to write that as:

if (!fndecl_nochk)
  continue;

fndecl = fndecl_nochk
gimple_call_set_fndecl (stmt, fndecl);



+		 to perform runtiome check for size and perform
s/runtiome/runtime/

+		 checks only when size is not zero.  */
+	      if (!known)
+		{
+		  gimple check = gimple_build_cond (NE_EXPR,
+						    size,
+						    size_zero_node,
+						    NULL_TREE,
+						    NULL_TREE);
+
+		  /* Split block before string function call.  */
+		  j = i;
+		  gsi_prev (&j);
+		  fall = split_block (bb, gsi_stmt (j));
+		  bb = fall->src;
+
+		  /* Add size check.  */
+		  j = gsi_last_bb (bb);
+		  if (gsi_end_p (j))
+		    gsi_insert_before (&j, check, GSI_SAME_STMT);
+		  else
+		    gsi_insert_after (&j, check, GSI_SAME_STMT);
+
+		  /* Create basic block for checks.  */
+		  check_bb = create_empty_bb (bb);
+		  make_edge (bb, check_bb, EDGE_TRUE_VALUE);
+		  make_single_succ_edge (check_bb, fall->dest, EDGE_FALLTHRU);
+
+		  /* Fix edge for splitted bb.  */
+		  fall->flags = EDGE_FALSE_VALUE;
+		  fall->count = bb->count;
+		  fall->probability = REG_BR_PROB_BASE;
+
+		  /* Update dominance info.  */
+		  if (dom_info_available_p (CDI_DOMINATORS))
+		    {
+		      set_immediate_dominator (CDI_DOMINATORS, check_bb, bb);
+		      set_immediate_dominator (CDI_DOMINATORS, fall->dest, bb);
+		    }
+
+		  /* Update loop info.  */
+		  if (current_loops)
+		    add_bb_to_loop (check_bb, bb->loop_father);
+
+		  /* Set position for checks.  */
+		  j = gsi_last_bb (check_bb);
+
+		  /* The block was splitted and therefore we
+		     need to set iterator to its end.  */
+		  i = gsi_last_bb (bb);
+		}
Basically we have a point (denoted by an iterator) and we want to split the block at that point and insert a conditional around some new blob of code before the original split point.

I'm a bit surprised we don't have this kind of capability already broken out. But assuming that's the case, can you go ahead and break that out into its own little helper function? You don't need to find all the cases where we're doing this kind of thing today, just create the helper function and use it in your new code.

+
+/* Finalise checker optimization  pass.  */
+void
+chkp_opt_fini (void)
+{
+  chkp_fix_cfg ();
+
+  free_dominance_info (CDI_DOMINATORS);
+  free_dominance_info (CDI_POST_DOMINATORS);
+}
You incrementally update the dominance information, so is there really a need to free it here? ie, the data is still valid, so I think we can just avoid the free_dominance_info calls.

Passes after this which use dominator information then won't have to rebuild it from scratch.

Generally this looks good.  With the light changes above, this will be OK.

Jeff


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]