On 12-10-15 12:49 PM, Richard Sandiford wrote:
Hi Vlad,
Some comments about the rest of LRA. Nothing major here...
Vladimir Makarov <vmaka...@redhat.com> writes:
+/* Info about register in an insn. */
+struct lra_insn_reg
+{
+ /* The biggest mode through which the insn refers to the register
+ (remember the register can be accessed through a subreg in the
+ insn). */
+ ENUM_BITFIELD(machine_mode) biggest_mode : 16;
AFAICT, this is actually always the mode of a specific reference,
and if there are references to the same register in different modes,
those references get their own lra_insn_regs. "mode" might be better
than "biggest_mode" if so.
I seems mode is also not good. I've just modified the comment to
reflect the fact that is just a reference.
+/* Static part (common info for insns with the same ICODE) of LRA
+ internal insn info. It exists in at most one exemplar for each
+ non-negative ICODE. Warning: if the structure definition is
+ changed, the initializer for debug_insn_static_data in lra.c should
+ be changed too. */
Probably worth saying (before the warning) that there is also
one structure for each asm.
Good point. I added a comment.
+/* LRA internal info about an insn (LRA internal insn
+ representation). */
+struct lra_insn_recog_data
+{
+ int icode; /* The insn code. */
+ rtx insn; /* The insn itself. */
+ /* Common data for insns with the same ICODE. */
+ struct lra_static_insn_data *insn_static_data;
Maybe worth mentioning asms here too.
Fixed.
+ /* Two arrays of size correspondingly equal to the operand and the
+ duplication numbers: */
+ rtx **operand_loc; /* The operand locations, NULL if no operands. */
+ rtx **dup_loc; /* The dup locations, NULL if no dups. */
+ /* Number of hard registers implicitly used in given call insn. The
+ value can be NULL or points to array of the hard register numbers
+ ending with a negative value. */
+ int *arg_hard_regs;
+#ifdef HAVE_ATTR_enabled
+ /* Alternative enabled for the insn. NULL for debug insns. */
+ bool *alternative_enabled_p;
+#endif
+ /* The alternative should be used for the insn, -1 if invalid, or we
+ should try to use any alternative, or the insn is a debug
+ insn. */
+ int used_insn_alternative;
+ struct lra_insn_reg *regs; /* Always NULL for a debug insn. */
Comments consistently above the field.
Fixed.
+extern void lra_expand_reg_info (void);
This doesn't exist any more.
Fixed.
+extern int lra_constraint_new_insn_uid_start;
Just saying in case: this seems to be write-only, with lra-constraints.c
instead using a static variable to track the uid start.
I realise you might want to keep it anyway for consistency with
lra_constraint_new_regno_start, or for debugging.
+extern rtx lra_secondary_memory[NUM_MACHINE_MODES];
This doesn't exist any more.
Removed. Thanks.
+/* lra-saves.c: */
+
+extern bool lra_save_restore (void);
Same for this file & function.
Removed.
+/* The function returns TRUE if at least one hard register from ones
+ starting with HARD_REGNO and containing value of MODE are in set
+ HARD_REGSET. */
+static inline bool
+lra_hard_reg_set_intersection_p (int hard_regno, enum machine_mode mode,
+ HARD_REG_SET hard_regset)
+{
+ int i;
+
+ lra_assert (hard_regno >= 0);
+ for (i = hard_regno_nregs[hard_regno][mode] - 1; i >= 0; i--)
+ if (TEST_HARD_REG_BIT (hard_regset, hard_regno + i))
+ return true;
+ return false;
+}
This is the same as overlaps_hard_reg_set_p.
I removed it and started to use the function overlaps_hard_reg_set_p.
+/* Return hard regno and offset of (sub-)register X through arguments
+ HARD_REGNO and OFFSET. If it is not (sub-)register or the hard
+ register is unknown, then return -1 and 0 correspondingly. */
The function seems to return -1 for both.
Fixed. It does not matter for the rest of code as offset is used only
when hard_regno >= 0.
+/* Add hard registers starting with HARD_REGNO and holding value of
+ MODE to the set S. */
+static inline void
+lra_add_hard_reg_set (int hard_regno, enum machine_mode mode, HARD_REG_SET *s)
+{
+ int i;
+
+ for (i = hard_regno_nregs[hard_regno][mode] - 1; i >= 0; i--)
+ SET_HARD_REG_BIT (*s, hard_regno + i);
+}
This is add_to_hard_reg_set.
Removed.
+ Here is block diagram of LRA passes:
+
+ ---------------------
+ | Undo inheritance | --------------- ---------------
+ | for spilled pseudos)| | Memory-memory | | New (and old) |
+ | and splits (for |<----| move coalesce |<-----| pseudos |
+ | pseudos got the | --------------- | assignment |
+ Start | same hard regs) |
---------------
+ | --------------------- ^
+ V | ---------------- |
+ ----------- V | Update virtual | |
+| Remove |----> ------------>| register | |
+| scratches | ^ | displacements | |
+ ----------- | ---------------- |
+ | | |
+ | V New |
+ ---------------- No ------------ pseudos -------------------
+ | Spilled pseudo | change |Constraints:| or insns | Inheritance/split |
+ | to memory |<-------| RTL |--------->| transformations |
+ | substitution | | transfor- | | in EBB scope |
+ ---------------- | mations | -------------------
+ | ------------
+ V
+ -------------------------
+ | Hard regs substitution, |
+ | devirtalization, and |------> Finish
+ | restoring scratches got |
+ | memory |
+ -------------------------
This is a great diagram, thanks.
+/* Create and return a new reg from register FROM corresponding to
+ machine description operand of mode MD_MODE. Initialize its
+ register class to RCLASS. Print message about assigning class
+ RCLASS containing new register name TITLE unless it is NULL. The
+ created register will have unique held value. */
+rtx
+lra_create_new_reg_with_unique_value (enum machine_mode md_mode, rtx original,
+ enum reg_class rclass, const char *title)
Comment says FROM, but parameter is called ORIGINAL. The code copes
with both null and non-register ORIGINALs, which aren't mentinoed in
the comment.
Fixed.
+/* Target checks operands through operand predicates to recognize an
+ insn. We should have a special precaution to generate add insns
+ which are frequent results of elimination.
+
+ Emit insns for x = y + z. X can be used to store intermediate
+ values and should be not in Y and Z when we use x to store an
+ intermediate value. */
I think this should say what Y and Z are allowed to be, since it's
more than just registers and constants.
Fixed.
+/* Map INSN_UID -> the operand alternative data (NULL if unknown). We
+ assume that this data is valid until register info is changed
+ because classes in the data can be changed. */
+struct operand_alternative *op_alt_data[LAST_INSN_CODE];
In that case I think it should be in a target_globals structure,
a bit like target_ira.
Fixed.
+ for (curr = list; curr != NULL; curr = curr->next)
+ if (curr->regno == regno)
+ break;
+ if (curr == NULL || curr->subreg_p != subreg_p
+ || curr->biggest_mode != mode)
+ {
+ /* This is a new hard regno or the info can not be
+ integrated into the found structure. */
+#ifdef STACK_REGS
+ early_clobber
+ = (early_clobber
+ /* This clobber is to inform popping floating
+ point stack only. */
+ && ! (FIRST_STACK_REG <= regno
+ && regno <= LAST_STACK_REG));
+#endif
+ list = new_insn_reg (regno, type, mode, subreg_p,
+ early_clobber, list);
+ }
+ else
+ {
+ if (curr->type != type)
+ curr->type = OP_INOUT;
+ if (curr->early_clobber != early_clobber)
+ curr->early_clobber = true;
+ }
OK, so this is probably only a technicality, but I think this should be:
for (curr = list; curr != NULL; curr = curr->next)
if (curr->regno == regno
&& curr->subreg_p == subreg_p
&& curr->biggest_mode == mode)
{
..reuse..;
return list;
}
..new entry..;
return list;
Fixed by using your approach. It cannot be return because the return
originally is after a loop covering this code.
+ icode = INSN_CODE (insn);
+ if (icode < 0)
+ /* It might be a new simple insn which is not recognized yet. */
+ INSN_CODE (insn) = icode = recog (PATTERN (insn), insn, 0);
Any reason not to use recog_memoized here?
It simply will call recog always as icode < 0. But as it has simpler
interface, I've changed it.
+ n = insn_static_data->n_operands;
+ if (n == 0)
+ locs = NULL;
+ else
+ {
+
+ locs = (rtx **) xmalloc (n * sizeof (rtx *));
+ memcpy (locs, recog_data.operand_loc, n * sizeof (rtx *));
+ }
Excess blank line after "else" (sorry!)
It looks like it is already fixed.
+ /* Some output operand can be recognized only from the context not
+ from the constraints which are empty in this case. Call insn may
+ contain a hard register in set destination with empty constraint
+ and extract_insn treats them as an input. */
+ for (i = 0; i < insn_static_data->n_operands; i++)
+ {
+ int j;
+ rtx pat, set;
+ struct lra_operand_data *operand = &insn_static_data->operand[i];
+
+ /* ??? Should we treat 'X' the same way. It looks to me that
+ 'X' means anything and empty constraint means we do not
+ care. */
FWIW, I think any X output operand has to be "=X" or "+X"; just "X"
would be as wrong as "r". genrecog is supposed to complain about that
for insns, and parse_output_constraint for asms.
So I agree the code is correct in just handling empty constraints.
Ok.
+/* Update all the insn info about INSN. It is usually called when
+ something in the insn was changed. Return the udpated info. */
Typo: updated.
Fixed.
+ for (i = 0; i < reg_info_size; i++)
+ {
+ bitmap_initialize (&lra_reg_info[i].insn_bitmap, ®_obstack);
+#ifdef STACK_REGS
+ lra_reg_info[i].no_stack_p = false;
+#endif
+ CLEAR_HARD_REG_SET (lra_reg_info[i].conflict_hard_regs);
+ lra_reg_info[i].preferred_hard_regno1 = -1;
+ lra_reg_info[i].preferred_hard_regno2 = -1;
+ lra_reg_info[i].preferred_hard_regno_profit1 = 0;
+ lra_reg_info[i].preferred_hard_regno_profit2 = 0;
+ lra_reg_info[i].live_ranges = NULL;
+ lra_reg_info[i].nrefs = lra_reg_info[i].freq = 0;
+ lra_reg_info[i].last_reload = 0;
+ lra_reg_info[i].restore_regno = -1;
+ lra_reg_info[i].val = get_new_reg_value ();
+ lra_reg_info[i].copies = NULL;
+ }
The same loop (with a different start index) appears in expand_reg_info.
It'd be nice to factor it out, so that there's only one place to update
if the structure is changed.
Fixed.
+ for (curr = data->regs; curr != NULL; curr = curr->next)
+ if (curr->regno == regno)
+ break;
+ if (curr->subreg_p != subreg_p || curr->biggest_mode != mode)
+ /* The info can not be integrated into the found
+ structure. */
+ data->regs = new_insn_reg (regno, type, mode, subreg_p,
+ early_clobber, data->regs);
+ else
+ {
+ if (curr->type != type)
+ curr->type = OP_INOUT;
+ if (curr->early_clobber != early_clobber)
+ curr->early_clobber = true;
+ }
+ lra_assert (curr != NULL);
+ }
Same loop comment as for collect_non_operand_hard_regs. Maybe another
factoring opportunity.
Fixed.
+ /* Some ports don't recognize the following addresses
+ as legitimate. Although they are legitimate if
+ they satisfies the constraints and will be checked
+ by insn constraints which we ignore here. */
+ && GET_CODE (XEXP (op, 0)) != UNSPEC
+ && GET_CODE (XEXP (op, 0)) != PRE_DEC
+ && GET_CODE (XEXP (op, 0)) != PRE_INC
+ && GET_CODE (XEXP (op, 0)) != POST_DEC
+ && GET_CODE (XEXP (op, 0)) != POST_INC
+ && GET_CODE (XEXP (op, 0)) != PRE_MODIFY
+ && GET_CODE (XEXP (op, 0)) != POST_MODIFY)
GET_RTX_CLASS (GET_CODE (XEXP (op, 0))) == RTX_AUTOINC
Fixed.
+/* Determine if the current function has an exception receiver block
+ that reaches the exit block via non-exceptional edges */
+static bool
+has_nonexceptional_receiver (void)
+{
+ edge e;
+ edge_iterator ei;
+ basic_block *tos, *worklist, bb;
+
+ /* If we're not optimizing, then just err on the safe side. */
+ if (!optimize)
+ return true;
+
+ /* First determine which blocks can reach exit via normal paths. */
+ tos = worklist = XNEWVEC (basic_block, n_basic_blocks + 1);
+
+ FOR_EACH_BB (bb)
+ bb->flags &= ~BB_REACHABLE;
+
+ /* Place the exit block on our worklist. */
+ EXIT_BLOCK_PTR->flags |= BB_REACHABLE;
+ *tos++ = EXIT_BLOCK_PTR;
+
+ /* Iterate: find everything reachable from what we've already seen. */
+ while (tos != worklist)
+ {
+ bb = *--tos;
+
+ FOR_EACH_EDGE (e, ei, bb->preds)
+ if (!(e->flags & EDGE_ABNORMAL))
+ {
+ basic_block src = e->src;
+
+ if (!(src->flags & BB_REACHABLE))
+ {
+ src->flags |= BB_REACHABLE;
+ *tos++ = src;
+ }
+ }
+ }
+ free (worklist);
+
+ /* Now see if there's a reachable block with an exceptional incoming
+ edge. */
+ FOR_EACH_BB (bb)
+ if (bb->flags & BB_REACHABLE)
+ FOR_EACH_EDGE (e, ei, bb->preds)
+ if (e->flags & EDGE_ABNORMAL)
+ return true;
+
+ /* No exceptional block reached exit unexceptionally. */
+ return false;
+}
Looks like we could just early out on the first loop and get rid
of the second.
It seems so. I fixed it. This is from reload. Code in reload could be
fixed too.
+/* Remove all REG_DEAD and REG_UNUSED notes and regenerate REG_INC.
+ We change pseudos by hard registers without notification of DF and
+ that can make the notes obsolete. DF-infrastructure does not deal
+ with REG_INC notes -- so we should regenerate them here. */
These days passes are supposed to regenerate REG_DEAD and REG_UNUSED
notes if they need them, so that part might not be necessary.
The REG_INC bit is still needed though...
Ok. I fixed it.
+/* Initialize LRA data once per function. */
+void
+lra_init (void)
+{
+ init_op_alt_data ();
+}
I think it's more like:
/* Initialize LRA whenever register-related information is changed. */
Fixed.
In summary, LRA looks really good to me FWIW. Thanks for all your hard work.
Getting rid of reload always seemed like a pipe dream, and if the only
known drawback of this replacement is that it takes a while on extreme
testcases, that's an amazing achievement. (Not to say compile time
isn't important, just that there were so many other hurdles to overcome.)
It is my second attempt. The first one was YARA project. I got a lot
of experience from this project and knowledge how not to do this.
LRA will be still a long lasting project. I don't think I found all
weirdness of reload just trying 8 targets (fixing one bug on one target
frequently resulted in new bugs on other targets so it required to do
frequently cardinal changes to the original code). Only after trying the
8 targets I got feeling that this approach could well. There are still
much more targets (and subtargets) to port to LRA. And I hope that
people will help me. I am very glad and appreciate that you did such
rigorous review because it means that you now understand LRA very well
(sometimes I think even better than me).
It looks like opinion has crystalised in favour of merging LRA for 4.8.
I hope that's what happens. I don't see that anything would be gained
by delaying it to 4.9. The code's not going to get any more testing on the
branch that it already has; whenever we merge, the stress test is always
going to be trunk.
Richard, thanks for you invaluable help.
