[PATCH 0/6] memops: small cleanups

[PATCH 0/6] memops: small cleanups

[Luc Van Oostenryck]

This series contains some small cleanups in preparation for
incoming changes concerning memops simplification

Luc Van Oostenryck (6):
  memops: dominates()s first arg is redundant
  memops: find_dominating_parents()s generation is redundant
  memops: remove obsolete comment
  memops: do not mess up with phisource's source ident
  memops: avoid using first_pseudo()
  memops: we can kill addresses unconditionally

 flow.c   |  4 ++--
 flow.h   |  2 +-
 memops.c | 39 ++++++++++++++++-----------------------
 3 files changed, 19 insertions(+), 26 deletions(-)


base-commit: 7b5cc7b6135733cbbce121cc94fdc4a5400f46b5
-- 
2.31.0

[PATCH 1/6] memops: dominates()s first arg is redundant

[Luc Van Oostenryck]

The first argument of dominates(), 'pseudo', is simply the 'src'
pseudo of it's second argument, the load or store instruction.

It's thus not needed to give it in a separate argument.

So, remove this redundant argument, since it makes things
slightly clearer.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 flow.c   |  4 ++--
 flow.h   |  2 +-
 memops.c | 12 ++++++------
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/flow.c b/flow.c
index cb94fcf20834..c235c5e92c8a 100644
--- a/flow.c
+++ b/flow.c
@@ -423,7 +423,7 @@ static inline int distinct_symbols(pseudo_t a, pseudo_t b)
  *
  * Return 0 if it doesn't, and -1 if you don't know.
  */
-int dominates(pseudo_t pseudo, struct instruction *insn, struct instruction *dom, int local)
+int dominates(struct instruction *insn, struct instruction *dom, int local)
 {
 	switch (dom->opcode) {
 	case OP_CALL: case OP_ENTRY:
@@ -440,7 +440,7 @@ int dominates(pseudo_t pseudo, struct instruction *insn, struct instruction *dom
 		return 0;
 	}
 
-	if (dom->src != pseudo) {
+	if (dom->src != insn->src) {
 		if (local)
 			return 0;
 		/* We don't think two explicitly different symbols ever alias */
diff --git a/flow.h b/flow.h
index c489ebe03034..d578fa95061a 100644
--- a/flow.h
+++ b/flow.h
@@ -41,7 +41,7 @@ static inline int kill_instruction_force(struct instruction *insn)
 }
 
 void check_access(struct instruction *insn);
-int dominates(pseudo_t pseudo, struct instruction *insn, struct instruction *dom, int local);
+int dominates(struct instruction *insn, struct instruction *dom, int local);
 
 extern void vrfy_flow(struct entrypoint *ep);
 extern int pseudo_in_list(struct pseudo_list *list, pseudo_t pseudo);
diff --git a/memops.c b/memops.c
index ff54208e2d54..7d9e4d703ee3 100644
--- a/memops.c
+++ b/memops.c
@@ -58,7 +58,7 @@ end:
 	repeat_phase |= REPEAT_CSE;
 }
 
-static int find_dominating_parents(pseudo_t pseudo, struct instruction *insn,
+static int find_dominating_parents(struct instruction *insn,
 	struct basic_block *bb, unsigned long generation, struct pseudo_list **dominators,
 	int local)
 {
@@ -75,7 +75,7 @@ static int find_dominating_parents(pseudo_t pseudo, struct instruction *insn,
 				continue;
 			if (one == insn)
 				goto no_dominance;
-			dominance = dominates(pseudo, insn, one, local);
+			dominance = dominates(insn, one, local);
 			if (dominance < 0) {
 				if (one->opcode == OP_LOAD)
 					continue;
@@ -90,7 +90,7 @@ no_dominance:
 			continue;
 		parent->generation = generation;
 
-		if (!find_dominating_parents(pseudo, insn, parent, generation, dominators, local))
+		if (!find_dominating_parents(insn, parent, generation, dominators, local))
 			return 0;
 		continue;
 
@@ -160,7 +160,7 @@ static void simplify_loads(struct basic_block *bb)
 				int dominance;
 				if (!dom->bb)
 					continue;
-				dominance = dominates(pseudo, insn, dom, local);
+				dominance = dominates(insn, dom, local);
 				if (dominance) {
 					/* possible partial dominance? */
 					if (dominance < 0)  {
@@ -180,7 +180,7 @@ static void simplify_loads(struct basic_block *bb)
 			generation = ++bb_generation;
 			bb->generation = generation;
 			dominators = NULL;
-			if (find_dominating_parents(pseudo, insn, bb, generation, &dominators, local)) {
+			if (find_dominating_parents(insn, bb, generation, &dominators, local)) {
 				/* This happens with initial assignments to structures etc.. */
 				if (!dominators) {
 					if (local) {
@@ -226,7 +226,7 @@ static void kill_dominated_stores(struct basic_block *bb)
 				int dominance;
 				if (!dom->bb)
 					continue;
-				dominance = dominates(pseudo, insn, dom, local);
+				dominance = dominates(insn, dom, local);
 				if (dominance) {
 					/* possible partial dominance? */
 					if (dominance < 0)
-- 
2.31.0

[PATCH 2/6] memops: find_dominating_parents()s generation is redundant

[Luc Van Oostenryck]

find_dominating_parents() has an argument 'generation' used
to check if a BB has already been visited.
But this argument is not needed since this generation is also
stored in the field ::generation of the current BB.

So, remove this argument.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 memops.c | 14 ++++++--------
 1 file changed, 6 insertions(+), 8 deletions(-)

diff --git a/memops.c b/memops.c
index 7d9e4d703ee3..753eb3a7a914 100644
--- a/memops.c
+++ b/memops.c
@@ -59,7 +59,7 @@ end:
 }
 
 static int find_dominating_parents(struct instruction *insn,
-	struct basic_block *bb, unsigned long generation, struct pseudo_list **dominators,
+	struct basic_block *bb, struct pseudo_list **dominators,
 	int local)
 {
 	struct basic_block *parent;
@@ -86,11 +86,11 @@ static int find_dominating_parents(struct instruction *insn,
 			goto found_dominator;
 		} END_FOR_EACH_PTR_REVERSE(one);
 no_dominance:
-		if (parent->generation == generation)
+		if (parent->generation == bb->generation)
 			continue;
-		parent->generation = generation;
+		parent->generation = bb->generation;
 
-		if (!find_dominating_parents(insn, parent, generation, dominators, local))
+		if (!find_dominating_parents(insn, parent, dominators, local))
 			return 0;
 		continue;
 
@@ -146,7 +146,6 @@ static void simplify_loads(struct basic_block *bb)
 			pseudo_t pseudo = insn->src;
 			int local = local_pseudo(pseudo);
 			struct pseudo_list *dominators;
-			unsigned long generation;
 
 			if (insn->is_volatile)
 				continue;
@@ -177,10 +176,9 @@ static void simplify_loads(struct basic_block *bb)
 			} END_FOR_EACH_PTR_REVERSE(dom);
 
 			/* OK, go find the parents */
-			generation = ++bb_generation;
-			bb->generation = generation;
+			bb->generation = ++bb_generation;
 			dominators = NULL;
-			if (find_dominating_parents(insn, bb, generation, &dominators, local)) {
+			if (find_dominating_parents(insn, bb, &dominators, local)) {
 				/* This happens with initial assignments to structures etc.. */
 				if (!dominators) {
 					if (local) {
-- 
2.31.0

[PATCH 3/6] memops: remove obsolete comment

[Luc Van Oostenryck]

The comment above rewrite_load_instruction(), about comparing phi-lists
for equality, was (most probably) written when there was some intention
to do CSE on phi-nodes or phi-sources.

However, such CSE is currently not an objective at all.

So, remove this comment.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 memops.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/memops.c b/memops.c
index 753eb3a7a914..5386c5a1f416 100644
--- a/memops.c
+++ b/memops.c
@@ -17,10 +17,6 @@
 #include "simplify.h"
 #include "flow.h"
 
-/*
- * We should probably sort the phi list just to make it easier to compare
- * later for equality.
- */
 static void rewrite_load_instruction(struct instruction *insn, struct pseudo_list *dominators)
 {
 	pseudo_t new, phi;
-- 
2.31.0

[PATCH 4/6] memops: do not mess up with phisource's source ident

[Luc Van Oostenryck]

In rewrite_load_instruction(), when testing if all phi-sources are
the same, the candidate is given an identifier if it hasn't one already.
But doing this inside this loop is strange:
* the pseudo may, at the end, not be selected but is changed anyway
* the identifier should be given either when the phi-source is created
  or at the end of the loop if selected.

So, do not change the identifier inside the selection loop.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 memops.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/memops.c b/memops.c
index 5386c5a1f416..119a39a180d5 100644
--- a/memops.c
+++ b/memops.c
@@ -29,7 +29,6 @@ static void rewrite_load_instruction(struct instruction *insn, struct pseudo_lis
 	FOR_EACH_PTR(dominators, phi) {
 		if (new != phi->def->phi_src)
 			goto complex_phi;
-		new->ident = new->ident ? : phi->ident;
 	} END_FOR_EACH_PTR(phi);
 
 	/*
-- 
2.31.0

[PATCH 5/6] memops: avoid using first_pseudo()

[Luc Van Oostenryck]

The loop in rewrite_load_instruction() uses first_pseudo() to not have
to special case the first element.

But this slightly complicates further changes.

So, simply use a null-or-no-null test inside the loop to identify
this first element.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 memops.c | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/memops.c b/memops.c
index 119a39a180d5..897fb6bf57fe 100644
--- a/memops.c
+++ b/memops.c
@@ -19,15 +19,17 @@
 
 static void rewrite_load_instruction(struct instruction *insn, struct pseudo_list *dominators)
 {
-	pseudo_t new, phi;
+	pseudo_t new = NULL;
+	pseudo_t phi;
 
 	/*
 	 * Check for somewhat common case of duplicate
 	 * phi nodes.
 	 */
-	new = first_pseudo(dominators)->def->phi_src;
 	FOR_EACH_PTR(dominators, phi) {
-		if (new != phi->def->phi_src)
+		if (!new)
+			new = phi->def->phi_src;
+		else if (new != phi->def->phi_src)
 			goto complex_phi;
 	} END_FOR_EACH_PTR(phi);
 
-- 
2.31.0

[PATCH 6/6] memops: we can kill addresses unconditionally

[Luc Van Oostenryck]

In rewrite_load_instruction(), if the load instruction is converted
into a phi-node, its address is then no more used and must be removed.

However, this is only done when this address is not a symbol.
This was explicitly done in the following commit because of the problem
of removing an element from the usage list while walking this list:
   602f6b6c0d41 ("Leave symbol pseudo usage intact when doing phi-node conversion.")

But currently rewrite_load_instruction() is only used during memops
simplification where the usage list is not walked.

So, kill the address' usage unconditionally.

Signed-off-by: Luc Van Oostenryck <luc.vanoostenryck@gmail.com>
---
 memops.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/memops.c b/memops.c
index 897fb6bf57fe..6d24604c2aae 100644
--- a/memops.c
+++ b/memops.c
@@ -45,9 +45,7 @@ static void rewrite_load_instruction(struct instruction *insn, struct pseudo_lis
 	goto end;
 
 complex_phi:
-	/* We leave symbol pseudos with a bogus usage list here */
-	if (insn->src->type != PSEUDO_SYM)
-		kill_use(&insn->src);
+	kill_use(&insn->src);
 	insn->opcode = OP_PHI;
 	insn->phi_list = dominators;
 
-- 
2.31.0