This is the mail archive of the
mailing list for the GCC project.
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
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
@@ -498,6 +519,10 @@ collect_pattern_seqs (void)
FOR_EACH_HTAB_ELEMENT (hash_bucket->seq_candidates, e0,
FOR_EACH_HTAB_ELEMENT (hash_bucket->seq_candidates, e1,
+ if (hti1.slot >= hti2.slot)
+ hti2.slot = hti1.slot;
if (e0 != e1
What does this do? Could use a comment.
This modify the 'hti2' iterator to speed up the inner cycle.
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. */
+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;
+ 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;
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
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)
- 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:
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.