Maciej Rozycki <maciej.rozy...@imgtec.com> writes:
> This however requires the correct annotation of branch targets as code,
> because the ISA mode is not relevant for data symbols and is therefore
> not recorded for them.
I wonder if it would have been possible to add the ISA mode to data
symbols and hide it in readelf/objdump? I don't know what older binutils
would have done with such labels but it would have made the new checks
compatible with pre-existing GCC code generation. Regardless the changes
in this patch would still be important to correctly identify labels as
text.
> Let it be produced then, making it appear in output generated right
> after `$L2' definitions above and thus fixing the assembly. Use the
> `mach2' pass, after all the MIPS16 constant pools have been fixed, to
> scan the insn stream backwards, identifying any labels still present at
> the end of a function or immediately preceding a MIPS16 constant pool,
> using dummy `consttable' insns previously inserted to identify the
> beginning of each such constant pool. Insert the `insn_pseudo' insn
> immediately after these labels, which emits the `.insn' pseudo-op.
>
> References:
>
> [1] "MIPS Architecture for Programmers, Volume II-B: The microMIPS32
> Instruction Set", MIPS Technologies, Inc., Document Number: MD00582,
> Revision 5.04, January 15, 2014, Section 7.1 "Assembly-Level
> Compatibility", p. 533
>
> [2] "MIPS Architecture for Programmers, Volume II-B: The microMIPS64
> Instruction Set", MIPS Technologies, Inc., Document Number: MD00594,
> Revision 5.04, January 15, 2014, Section 8.1 "Assembly-Level
> Compatibility", p. 623
>
> gcc/
> * config/mips/mips.c (mips16_emit_constants): Emit `consttable'
> insn at the beginning of the constant pool.
> (mips_insert_insn_pseudos): New function.
> (mips_machine_reorg2): Call it.
> * config/mips/mips.md (unspec): Add UNSPEC_CONSTTABLE and
> UNSPEC_INSN_PSEUDO enum values.
> (insn_pseudo, consttable): New insns.
>
> gcc/testsuite/
> * gcc.target/mips/insn-casesi.c: New test case.
> * gcc.target/mips/insn-pseudo-1.c: New test case.
> * gcc.target/mips/insn-pseudo-2.c: New test case.
> * gcc.target/mips/insn-pseudo-3.c: New test case.
> * gcc.target/mips/insn-pseudo-4.c: New test case.
> * gcc.target/mips/insn-tablejump.c: New test case.
> ---
> I have successfully regression-tested it with the `mips-mti-linux-gnu'
> target, with a big-endian o32 regular MIPS multilib and a little-endian
> o32 MIPS16 multilib, with no regressions, except as noted below. I did
> some big-endian n64 regular MIPS and little-endian o32 microMIPS
> testing, including with the new cases, and things looked good, except as
> noted below. I also generated assembly manually (for the assembly-match
> cases) and examined output visually, including all the four above
> multilibs, and also -fPIC and -mno-abicalls variants, which I have no
> immediate way of testing automatically.
As noted below and my opinion in general... Dealing with the intricacies
of getting the MIPS part of the GCC testsuite running cleanly for all
variations of the architecture is a prohibitively expensive process to
apply to each patch. Now that we are in stage 3 then various testsuite
issues will get dealt with.
> With n64 (`-mabi=n64') testing none of the test cases under
> gcc.target/mips/ were run and the test harness broke as follows:
>
> ERROR: (DejaGnu) proc "cc1: error: '-mfpxx' can only be used with the
> o32 ABI" does not exist.
> The error code is NONE
> The info on the error is:
> invalid command name "cc1:"
> while executing
> "::tcl_unknown cc1: error: '-mfpxx' can only be used with the o32 ABI"
> ("uplevel" body line 1)
> invoked from within
> "uplevel 1 ::tcl_unknown $args"
>
> I take it as a bug in the harness, which ought to be looked into
> separately, and not a problem with this change.
I haven't seen this failure before which may be down to a different way
of invoking the testsuite I guess (I have run n64 testing fairly recently).
> With MIPS16 (`-mips16') and microMIPS (`-mmicromips') testing the test
> cases failed to compile as follows, respectively:
>
> spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/
> .../gcc/testsuite/gcc.target/mips/insn-tablejump.c -fno-diagnostics-
> show-caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 -
> DNOMIPS16=__attribute__((nomips16)) -
> DNOMICROMIPS=__attribute__((nomicromips)) -
> DNOCOMPRESSION=__attribute__((nocompression)) -mmicromips -mno-mips16 -
> DMICROMIPS=__attribute__((micromips)) -EL -mips16 -Wl,-dynamic-
> linker,.../sysroot/mipsel-r2-mips16-hard/lib/ld.so.1 -Wl,-
> rpath,.../sysroot/mipsel-r2-mips16-hard/lib -Wl,-
> rpath,.../sysroot/mipsel-r2-mips16-hard/usr/lib -lm -o insn-
> tablejump.exe
> cc1: error: unsupported combination: -mips16 -mmicromips compiler exited
> with status 1 output is:
> cc1: error: unsupported combination: -mips16 -mmicromips
>
> FAIL: gcc.target/mips/insn-tablejump.c -O0 (test for excess errors)
> Excess errors:
> cc1: error: unsupported combination: -mips16 -mmicromips
>
> and:
>
> spawn -ignore SIGHUP .../gcc/gcc/xgcc -B.../gcc/gcc/
> .../gcc/testsuite/gcc.target/mips/insn-casesi.c -fno-diagnostics-show-
> caret -fdiagnostics-color=never --sysroot=.../sysroot -O0 -
> DNOMIPS16=__attribute__((nomips16)) -
> DNOMICROMIPS=__attribute__((nomicromips)) -
> DNOCOMPRESSION=__attribute__((nocompression)) -mno-micromips -mips16 -
> mcode-readable=yes -DMIPS16=__attribute__((mips16)) -EL -mmicromips-Wl,-
> dynamic-linker,.../sysroot/micromipsel-r2-hard/lib/ld.so.1 -Wl,-
> rpath,.../sysroot/micromipsel-r2-hard/lib -Wl,-
> rpath,.../sysroot/micromipsel-r2-hard/usr/lib -lm -o insn-casesi.exe
> cc1: error: unsupported combination: -mips16 -mmicromips compiler exited
> with status 1 output is:
> cc1: error: unsupported combination: -mips16 -mmicromips
>
> FAIL: gcc.target/mips/insn-casesi.c -O0 (test for excess errors)
> Excess errors:
> cc1: error: unsupported combination: -mips16 -mmicromips
>
> Again, I take it as a bug in the harness, which places the required
> overrides specified and implied by `dg-options' before board compilation
> flags rather than afterwards. As such it's a separate problem which has
> nothing to do with this change and needs to be fixed independently.
I see nothing wrong with the way the tests are described so it does look
like a harness issue. There are tests that fall into each of the two
categories of failure but as above I don't recall seeing these kind of
failures before.
> OK to apply?
OK, thanks.
Matthew
