On Sat, Nov 10, 2018 at 11:36:28AM -0600, Kelvin Nilsen wrote:
> This new pass scans existing rtl expressions and replaces them with rtl
> expressions that favor selection of the D-form instructions in contexts for
> which the D-form instructions are preferred.
(Here would be a good place to say it runs right after the RTL loop opts).
> Both regressions relate to resolution of ifuncs, and I have determined that
> the toc pointer upon entry into the resolver functions are not valid. I have
> not yet determined why this is happening, though I have observed that the
> same problem seems to occur with certain other versions of the compiler prior
> to my trunk with patch. The two failures are:
>
> FAIL: gcc.dg/attr-ifunc-4.c execution test
> FAIL: gcc.dg/ipa/ipa-pta-19.c execution test
It sounds like those are not new failures. Would be good to figure out
what causes it of course.
> * config/rs6000/rs6000-p9indexing.c: New file.
A better name would be good :-)
> +bool
> +rs6000_target_supports_dform_offset_p (bool is_store __attribute__((unused)),
Just say bool /*is_store*/ or completely delete this if it isn't used?
Shorter name wouldn't hurt either ;-)
> +{
> + const int max_16bit_signed = (0x7fffffff);
> + const int min_16bit_signed = -1 - max_16bit_signed;
Please use HOST_WIDE_INT, not int.
> + /* available d-form instructions with P1 (the original Power architecture):
We don't actually support that anymore. But we do support original PowerPC
still, which matches what you say here (POWER used diffent mnemonics).
> + /* available d-form instructions with PPC (prior to v2.00):
This is the 64-bit instructions (64-bit is not required before ISA 3.0).
Those did not exist before PowerPC either, yup.
> + if ((byte_offset >= min_16bit_signed)
> + && (byte_offset <= max_16bit_signed))
if (IN_RANGE (byte_offset, min_16bit_signed, max_16bit_signed))
> + return true;
> + else
> + break;
Don't say "return else break" please, just say "return; break;"
> + if (rs6000_isa_flags & OPTION_MASK_MODULO)
> + { /* ISA 3.0 */
Don't put the "ISA" comment here please, put it somewhere before the "{".
> + case E_DFmode:
> + case E_SFmode:
> + /* E_DFmode handled by lxsd and stxsd insns. E_SFmode handled
> + by lxssp and stxssp insn. */
These are already handled by {l,st}f[sd] which are base PowerPC, at any
offset (not just multiples of 4)?
> --- gcc/config/rs6000/rs6000-passes.def (revision 263589)
> +++ gcc/config/rs6000/rs6000-passes.def (working copy)
> @@ -25,3 +25,4 @@
> */
>
> INSERT_PASS_BEFORE (pass_cse, 1, pass_analyze_swaps);
> + INSERT_PASS_AFTER (pass_loop2, 1, pass_fix_indexing);
This name is not super.
> \ No newline at end of file
And neither is that :-)
> + /* If this insn is relevant, it belongs to an equivalence class.
> + The equivalence classes are identified by the definitions that
> + define the inputs to this insn.
> + */
No new line for the comment end please.
> + unsigned int is_relevant: 1;
A space before the colon, too.
> +static int count_links (struct df_link *def_link)
> +{
> + if (def_link == NULL) return 0;
> + else return 1 + count_links (def_link->next);
Each return should start a new line.
> +static int
> +help_hash (int count, struct df_link *def_link) {
> + int *ids;
> + int i = 0;
> +
> + if (count > max_use_links)
> + max_use_links = count;
> +
> + ids = (int *) alloca (count * sizeof (int));
sizeof *ids; but can't you use a VLA?
> + while (def_link != NULL) {
{ on a new line, indented.
> + /* bubble sort to put ids in ascending order. */
> + for (int end = count - 1; end > 0; end--) {
> + for (int j = 0; j < end; j++) {
> + if (ids[j] > ids[j+1]) {
> + int swap = ids[j];
> + ids[j] = ids[j+1];
> + ids[j+1] = swap;
std::swap to swap. And just use qsort?
> +static void
> +find_defs (indexing_web_entry *insn_entry, rtx_insn *insn,
> + struct df_link **insn_base, struct df_link **insn_index)
> +{
> + unsigned int uid = INSN_UID (insn);
> + rtx body = PATTERN (insn);
> + rtx mem;
> + if ((GET_CODE (body) == SET) && MEM_P (SET_SRC (body)))
> + mem = XEXP (SET_SRC (body), 0);
> + else if ((GET_CODE (body) == SET) && MEM_P (SET_DEST (body)))
> + mem = XEXP (SET_DEST (body), 0);
> + else
> + mem = NULL;
> + /* else, this insn is neither load nor store. */
Use single_set instead?
> + }
> + }
> + }
> + }
> + }
> + }
> + }
> +}
This means you need some more factoring and/or some early outs.
> +/* Return non-zero if an only if use represents a compile-time constant. */
> +static int
> +represents_constant_p (df_ref use)
You're returning true or false, so please declare this s "bool".
> + /* We're only happy with multiple uses if all but one represent
> + constant values. Below, we assure that */
That what? Or missing full stop?
> + if (non_constant_uses == 1) {
{ on a new line, indented, and indent after this needs some work.
> + else /* assumption is that *adjustment is
> + added to a positive variable
> + expression, so don't optimize this
> + rare condition. */
Please don't write comments like this. Just start them on their own line,
at the indent of what they belong to.
> +/* Return true iff instruction I2 dominates instruction I1. */
> +static bool
> +insn_dominated_by_p (rtx_insn *i1, rtx_insn *i2)
> +{
> + basic_block bb1 = BLOCK_FOR_INSN (i1);
> + basic_block bb2 = BLOCK_FOR_INSN (i2);
> +
> + if (bb1 == bb2)
> + {
> + for (rtx_insn *i = i2; BLOCK_FOR_INSN (i) == bb1; i = NEXT_INSN (i))
> + if (i == i1)
> + return true;
> + return false;
> + }
> + else
> + return dominated_by_p (CDI_DOMINATORS, bb1, bb2);
> +}
Strange that there is nothing for this yet, or some "this insn precedes
that insn" function.
NEXT_INSN can return NULL, and BLOCK_FOR_INSN doesn't like that.
> +/* Main entry point for this pass.
> +
> + This pass runs after loops have been unrolled. The pass searches
> + for sequences of code of the form:
Please put this comment at the start of the file?
And then later on you have some functions that use more than half of the
available horizontal space (80 chars) for indent. You need more factoring ;-)
Should this pass be after the loop opts? Should it be right after expand,
instead? Or elsewhere? If it mostly fixes up bad ivopts decisions, is
there something that can be done about that?
Some examples would be good, also in the comment at start of file.
Segher
