On 4/16/23 07:20, Ajit Agarwal wrote:
Hello All:
This patch improves code sinking pass to sink the blocks before calls
in the use blocks or immediate dominator blocks that reduces register pressure.
Bootstrapped and regtested on powerpc64-linux-gnu.
Thanks & Regards
Ajit
tree-ssa-sink: Improve code sinking pass.
Code Sinking sinks the blocks after call. This increases
register pressure for callee-saved registers. Improves
code sinking before call in the use blocks or immediate
dominator of use blocks.
2023-04-16 Ajit Kumar Agarwal <aagar...@linux.ibm.com>
gcc/ChangeLog:
* tree-ssa-sink.cc (statement_sink_location): Modifed to
move statements before calls.
(block_call_p): New function.
(def_use_same_block): New function.
(select_best_block): Add heuristics to select the best
blocks in the immediate post dominator.
gcc/testsuite/ChangeLog:
* gcc.dg/tree-ssa/ssa-sink-20.c: New testcase.
* gcc.dg/tree-ssa/ssa-sink-21.c: New testcase.
---
+/* Check def and use stmts are in same block. */
A better function comment would be
/* Return TRUE if all the immediate uses of the defs in
USE occur in the same block as USE, FALSE otherwise. */
I would also strongly suggest you change "use" to something else. This
function is walking over uses and defs, so calling the incoming argument
"use" is going to make it excessively hard to write clean comments for
this function. Something as simple as "stmt" would be considerably better.
+
+bool
+def_use_same_block (gimple *use)
+{
+ use_operand_p use_p;
+ def_operand_p def_p;
+ imm_use_iterator imm_iter;
+ ssa_op_iter iter;
+
+ FOR_EACH_SSA_DEF_OPERAND (def_p, use, iter, SSA_OP_DEF)
+ {
+ FOR_EACH_IMM_USE_FAST (use_p, imm_iter, DEF_FROM_PTR (def_p))
+ {
+ if (is_gimple_debug (USE_STMT (use_p)))
+ continue;
+
+ if (use_p
+ && (gimple_bb(USE_STMT (use_p)) == gimple_bb (use)))
Minor whitespace problems. Make sure to include a space between the
function name you are calling and the open parenthesis for the
arguments. ie gimple_bb (USE_STMT (use_p)).
It also seems like you're not checking all the uses, just one of them?
Is that what you intended? If so, then my suggested function comment is
wrong and needs further adjustment. This highlights how important it is
to have a good function comment.
+/* Check if the block has only calls. */
This comment doesn't match the code. It appears that you can have both
calls and conditional branches. Please update the function comment
appropriately. You should also describe the arguments and return value
in the function comment (see my suggestion above as an example for how
to describe the function arguments and return value.
Based on the code it looks like you're requiring a the block to contain
only two real statements. A call followed by a conditional.
+
+bool
+block_call_p (basic_block bb)
+{
+ int i = 0;
+ bool is_call = false;
+ gimple_stmt_iterator gsi = gsi_last_bb (bb);
+ gimple *last_stmt = gsi_stmt (gsi);
ISTM there is likely a function that will give you the last statement in
the function.
+
+ if (last_stmt && gimple_code (last_stmt) == GIMPLE_COND)
+ {
+ if (!gsi_end_p (gsi))
+ gsi_prev (&gsi);
+
+ for (; !gsi_end_p (gsi);)
+ {
+ gimple *stmt = gsi_stmt (gsi);
+
+ if (is_gimple_debug (stmt))
+ return false;
Definitely incorrect as this can cause the decisions we make for
optimization to change based on the existence of debug statements.
+
+ if (is_gimple_call (stmt))
+ is_call = true;
+ else
+ return false;
ISTM that this might be better/clearer. Once you've seen a call, if you
see another, you can just return immediately. It also seems like if I
ever has a value other than 0/1, then you can return false immediately.
if (is_gimple_call (stmt))
{
/* We have already seen a call. */
if (is_call)
return false;
is_call = true;
continue;
}
+
+ if (!gsi_end_p (gsi))
+ gsi_prev (&gsi);
+
+ ++i;
Isn't this going to cause this routine to return false if it has (for
example) one or more labels followed by a CALL, then a conditional?
Overall I think the logic in here needs a bit of work.
@@ -190,7 +254,8 @@ nearest_common_dominator_of_uses (def_operand_p def_p, bool
*debug_stmts)
static basic_block
select_best_block (basic_block early_bb,
basic_block late_bb,
- gimple *stmt)
+ gimple *stmt,
+ gimple *use = 0)
Rather than use a default value, just fix the callers. There's only 3
and you already fixed one :-) And if you're going to initialize a
pointer, use NULL rather than 0.
{
basic_block best_bb = late_bb;
basic_block temp_bb = late_bb;
@@ -230,7 +295,28 @@ select_best_block (basic_block early_bb,
if (threshold > 100)
threshold = 100;
}
+ if (bb_loop_depth (best_bb) == bb_loop_depth (early_bb)
+ && !(best_bb->count * 100 >= early_bb->count * threshold))
Isn't this condition the same as the one immediately following? Note
how there's a nice comment before the next one indicating what the
heuristic is? That's the kind of thing we want for every decision we
make in this function. In fact, I think there's a comment describing
every block selection in this function.
+ {
+ basic_block new_best_bb = get_immediate_dominator (CDI_DOMINATORS,
best_bb);
+
+ if (new_best_bb && use
+ && (new_best_bb != best_bb)
+ && (new_best_bb != early_bb)
+ && !is_gimple_call (stmt)
+ && gsi_end_p (gsi_start_phis (new_best_bb))
+ && (gimple_bb (use) != early_bb)
+ && !is_gimple_call (use)
+ && dominated_by_p (CDI_POST_DOMINATORS, new_best_bb, gimple_bb(use))
+ && dominated_by_p (CDI_DOMINATORS, new_best_bb, early_bb)
+ && block_call_p (new_best_bb))
So in english, what property are you checking for in this series of tests?
if (sinkbb == frombb)
return false;
- if (sinkbb == gimple_bb (use))
- *togsi = gsi_for_stmt (use);
- else
- *togsi = gsi_after_labels (sinkbb);
+ gimple *def_stmt = SSA_NAME_DEF_STMT (DEF_FROM_PTR (def_p));
[ ... ]
So, there's no comments in this code describing what you're looking for
or why.
There are numerous comments in this file to help guide you with what you
should be adding as comments. The key is to make it so that someone not
real familiar with this code/file should be able to read the comments
and have a general understanding of what a hunk of code is doing and why.
What kind of testing have you done to make sure this doesn't change the
sinking behavior for codes other than the case you're trying to improve.
RIght now I can't convince myself that your patch isn't going to have
unintended consequences from a code generation standpoint.
Jeff
