Re: [RFA] PowerPC VLE port

[Alan Modra <amodra at gmail dot com>]

On Fri, Aug 12, 2011 at 02:05:46PM -0400, Catherine Moore wrote:
> Tests for binutils/gas/ld testsuites for the ppc-eabi, power64,
> powerpc-aix, powerpc-linux, powerpc-rtems, and powerpc-sysv4
> configurations reported no regressions.  If I've missed an important
> configuration for testing, please let me know.

I'd like to see you do a 32-bit powerpc-linux gcc bootstrap and
regression test using binutils with your patch applied, and compare
results with the same using baseline binutils.  Comparing all object
files in the trees would also give me a feeling of security.

> 	(DWARF2_LINE_MIN_INSN_LENGTH): Redefine to 2.

Of course, comparing object files won't work with the current patch.
This needs fixing.  I'm not sure, but I think
DWARF2_LINE_MIN_INSN_LENGTH is only used after gas has finished
reading the input file, so you can select 2 if a 2 byte VLE insn has
been used anywhere in the file.

-/* This bit is reserved by BFD for processor specific stuff.  Name
-   it properly so that we can easily stay consistent elsewhere.  */
-#define SEC_PPC_VLE		SEC_TIC54X_BLOCK

Please revert this accidental commit now rather than waiting until the
whole patch is approved.

+/* Processor specific program headers, p_flags field.  */
+#define PF_PPC_VLE		0x10000000	/* PowerPC VLE.  */

Used where?

> -      table_op = PPC_OP (opcode->opcode);
> -      if (op < table_op)
> +      mask = opcode->mask;
> +      table_opcode = opcode->opcode;
> +      table_op_is_short = PPC_OP_SE_VLE (mask);
> +      table_op = PPC_OP (table_opcode, PPC_OP_SA (mask));
> +
> +      masked_op = op;
> +      if (table_op_is_short)
> +        {
> +          /* Some short instructions only have 4 or 5 opcode bits.  */
> +          if ((mask & 0xfff) == 0)
> +            masked_op &= 0x3c;
> +          else if ((mask & 0x7ff) == 0)
> +            masked_op &= 0x3e;
> +        }
> +      if (masked_op < table_op)
>  	break;
> -      if (op > table_op)
> +      if (masked_op > table_op)
>  	continue;

Hmm.  How much does this patch slow down objdump -d of large powerpc
object files?  If significant, can you do something about it?

> +          /* Skip se_bc in favor of simplified mnemonics.  */
> +          if (table_opcode == 0xe000 && !strcmp (opcode->name, "se_bc"))
> +            continue;

Sort the opcode table differently instead?

> +      /* Skip e_bc or e_bcl in favor of simplified mnemonics.  */
> +      else if (table_opcode == 0x7a000000 && !strcmp (opcode->name, "e_bc"))
> +          continue;
> +      else if (table_opcode == 0x7a000001 && !strcmp (opcode->name, "e_bcl"))
> +          continue;

Ditto.

> -	    (*info->fprintf_func) (info->stream, "%ld", value);
> +	    (*info->fprintf_func) (info->stream, "%d", value);

value is a long isn't it?

> +  /* A relative 8 bit branch.  */
> +  HOWTO (R_PPC_VLE_REL8,	/* type */
> +	 1,			/* rightshift */
> +	 1,			/* size (0 = byte, 1 = short, 2 = long) */
> +	 8,			/* bitsize */
> +	 TRUE,			/* pc_relative */
> +	 0,			/* bitpos */
> +	 complain_overflow_bitfield, /* complain_on_overflow */

complain_overflow_signed?  Ditto other relative branches.

> +static bfd_reloc_status_type
> +ppc_elf_vle_addr16_split16a (bfd *abfd ATTRIBUTE_UNUSED,

Unfinished?  I don't see anything to insert the split field, and the
function is identical to..

> +static bfd_reloc_status_type
> +ppc_elf_vle_addr16_split16d (bfd *abfd ATTRIBUTE_UNUSED,

..this one.

> @@ -3502,6 +3903,9 @@
>  
>        switch (r_type)
>  	{
> +	default:
> +	  break;
> +

Please don't add default cases to switch statements that previously
lacked them.  I like a warning if some relocation type is missing
in check_relocs.

> +.  {*  Indicate that the section has the VLE bit set. *}
> +.#define SEC_PPC_VLE 0x80000000
> +.

Where is this used?  If needed, I'd rather see one of the sec_flg[0-6]
bits used instead for backend specific flags.

-- 
Alan Modra
Australia Development Lab, IBM