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: [PING] Re: [Patch] Fix the allocation of jump register for RTL sequence abstraction


Gabor Loki wrote:

We need to redesign this function, because the modified 'compute_init_costs'
function could generate false hash buckets when HASHED_INSNS is too
small. This is not so critical as the following code, but it could lead to false results.

How can this happen? This problem should be documented somewhere in the code.



@@ -498,6 +519,10 @@ collect_pattern_seqs (void)
FOR_EACH_HTAB_ELEMENT (hash_bucket->seq_candidates, e0, p_hash_elem, hti1)
FOR_EACH_HTAB_ELEMENT (hash_bucket->seq_candidates, e1, p_hash_elem,
hti2)
+ {
+ if (hti1.slot >= hti2.slot)
+ hti2.slot = hti1.slot;
+ else
if (e0 != e1


What does this do? Could use a comment.

This modify the 'hti2' iterator to speed up the inner cycle.


like:
for (,,i++)
       for (j=i,,j++)

Depends on iterator and hash table internals which kind of breaks the abstraction. Better to create a new FOR_EACH_HTAB_ELEMENT_FROM with an extra argument? That's preapproved as a separate patch.


+/* Creates a jump insn for abstraction.  */
+
+static rtx
+seq_make_jump_insn_raw (rtx insn)
+{
+  rtx new_insn;
+
+  switch (GET_CODE (insn))
+    {
+    case INSN:
+    case JUMP_INSN:
+    case CALL_INSN:
+    case CODE_LABEL:
+    case BARRIER:
+    case NOTE:
+      new_insn = insn;
+      break;


+    default:
+      new_insn = make_jump_insn_raw (insn);
+    }


This looks odd. Can you really pass both patterns and whole insns into this? Do you really need to handle NOTEs and INSNs?

Yes, we need. We don't know the results of 'gen_jump' and 'gen_indirect_jump' (which are the inputs of this function). It could be a pattern (like x86) or a sequence (like sh) too.

But all the following code assumes that you only get a single insn, starting with the setting of NEXT_INSN/PREV_INSN. If you get a single one, it must be a JUMP_INSN. Better return NULL and abort the optimization if you get anything other than a jump pattern/insn.


+  NEXT_INSN (new_insn) = new_insn;
+  PREV_INSN (new_insn) = new_insn;

Why?

H mm. I should had to place a comment. This is not a trivial solution. The algorithm creates some temporary insns which are used to measure the initial costs ('compute_init_costs' function), but the cost computation only works for valid insns. So we need to decorate the temporary insns to look like valid.

If you're only computing costs you shouldn't need anything other than a pattern.
Hmm, let me see if I understand all this correctly. It goes through compute_rtx_cost, which in turn tries to look it up in a hash table using compute_hash, which looks at PREV_INSN. In fact, with the patch it'll now loop through the same insn for HASHED_INSN times?
At the end it'll call get_attr_length which is likely to be unreliable for a newly generated indirect jump using a pseudo. Won't that cause a problem once get_attr_length accesses INSN_ADDRESSES for the new insn?
I still don't see why it needs NEXT_INSN/PREV_INSN.


It is conservative approach. The 'get_indirect_jump_insn_no'
function returns the result of 'recog_memoized' which could
return with negative value.

An expander like gen_indirect_jump isn't supposed to produce an invalid insn if the inputs match the predicates. If really you want the sanity check, put a gcc_unreachable in there - that way we'll know immediately when something isn't as it should be.


@@ -1411,7 +1662,7 @@ rtl_seqabstr (void)
 static bool
 gate_rtl_seqabstr (void)
 {
-  return flag_rtl_seqabstr;
+  return flag_rtl_seqabstr && optimize_size;
 }


This doesn't look right, why prevent people from using -O2 -frtl-seqabstr? Not part of the bugfix either.

Maybe this is not the part of jump register bugfix, but it is also important. We have a small discussion about cost computing of the algorithm earlier. The final conclusion was that the abstraction doesn't run when -Os isn't specified. Please see the following threads: - http://gcc.gnu.org/ml/gcc-patches/2005-12/threads.html#00988 - http://gcc.gnu.org/ml/gcc-patches/2006-01/threads.html#00331

Ah. It helps to repeat these things when submitting a patch to prevent the reviewer from having to guess the purpose of a change. This part is ok separately, although there is room for improvement - we really should be able to estimate insn lengths at all times, even with -O2.



Bernd



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