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][SMS] SMS use loop induction variable analysis instead of depending on doloop optimization


On 04/28/2016 08:06 AM, Shiva Chen wrote:

Could anyone help me to review the patch?
Any suggestion would be very helpful.

You might want to split it up if there are several logically independent pieces. I can't quite make sense of it all, and I'm not too familiar with SMS anyway, so the following is not a complete review, just a selection of issues I observed.

There are a large number of formatting and style problems. I'll be pointing out some instances, but please read
   http://www.gnu.org/prep/standards/standards.html#Writing-C
and self-review your patch before resubmission.

+static bool mem_write_insn_p (rtx_insn *);

Generally best to order your code so that you don't need forward declarations.

-  /* We do not handle setting only part of the register.  */
-  if (DF_REF_FLAGS (adef) & DF_REF_READ_WRITE)
-    return GRD_INVALID;
-

Why this change?

  }

+static rtx
+get_rhs (rtx_insn *insn, rtx reg)

get_rhs might not be the most meaningful function name. We require documentation before every function that says what it does, and what the arguments mean. Please examine the surrounding code for examples.

+
+  /* Find iv increment/decrement rhs in following pattern
+
+     (parallel [
+	(set (reg:CC_NOOV 100 cc)
+	    (compare:CC_NOOV (plus:SI (reg:SI 147)
+				      (const_int -1))
+			     (const_int 0 [0])))
+	(set (reg:SI 147)
+	    (plus:SI (reg:SI 147)
+		     (const_int -1 [0xffffffffffffffff]))
+   */

Rather than quoting large RTL blocks, it would be better to explain what you're trying to do.

@@ -1048,6 +1141,47 @@ iv_analyze_expr (rtx_insn *insn, rtx rhs, machine_mode mode,
    return iv->base != NULL_RTX;
  }

+/* Auxiliary variable for mem_read_insn_p/mem_write_insn_p.  */
+static bool mem_ref_p;

Auxiliary variable doing what? Rather than using a global, it might be better to use the data pointer passed through note_uses.

+   /* To check the case REG is read write register
+      in memory reference.  */
+  if (mem_write_insn_p (insn))
+    body = SET_DEST (set);
+  else if (mem_read_insn_p (insn))
+    body = SET_SRC (set);

This all looks a little odd; if you're looking for autoincs, why not just scan the entire INSN for a MEM, rather than classify it as mem_write or mem_read_insn?

+      if (GET_CODE (body) == ZERO_EXTEND ||
+	  GET_CODE (body) == SIGN_EXTEND)

Split lines before operators.

+
+  /* To handle the condition as follow
+      (ne (plus:SI (reg:SI 163)
+		   (const_int -1))
+	  (const_int 0 [0]))
+
+     The pattern would generate
+     by doloop optimization.  */

This comment confuses me more than it helps me.

+
+  /* handle the condition:
+  (ne (plus:SI (reg:SI 163)
+               (const_int -1 [0xffffffffffffffff]))
+       (const_int 0 [0]))
+  */

Handle how? This has comment formatting problems, which would be easier to make go away if you didn't just quote RTL. (If you must quote RTL, best to remove irrelevant bits such as :SI and [0xfffff....] and [0]), and replace register numbers like 163 with variables like A.

+
+  /* following code handle the condition:
+     (ne (reg:SI 163) (reg:SI 176))
+  */

Same problem.

+	  if ((set = single_set (insn)))
> +	    {
> +	      if (SET_DEST (set) == count_reg)
> +		continue;
> +	      else
> +		{
> +		  loop->count_ref_p = true;
> +		  return;
> +		}
> +	    }

Either
	  if ((set = single_set (insn)) != NULL_RTX)
or, better (also removing the outer declaration of set):

    rtx set = single_set (insn);
    if (set && SET_DEST (set) == count_reg)
      continue;
    loop->count_ref_p = true;
    return;

+
+	  if (GET_CODE (PATTERN (insn)) == PARALLEL)
+	    {

No unnecessary braces if an if contains only a single statement.

+  /* check count_reg reference in the loop
+     and set result to loop->count_ref_p.  */
+  check_count_reg_reference (loop, iv->base);

Comments should be full sentences, properly capitalized. But avoid comments that just describe what a called function is doing. This information should be part of that function's starting comment.

Skipping most of the rest of the SMS changes...


Bernd


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