This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: gcc-3.4: MaverickCrunch coprocessor bugfixes (EP9312)


On Fri, 5 Mar 2004, Richard Earnshaw wrote:

> How is that copyright assignment coming on?  We can't do anything with
> this until that is sorted.

We're in process of obtaining one, however we could use the opportunity to
reach an agreement on this patch in the meantime.

> I would note that I think it's already way to late to put this into 3.4.0,
> which is due to enter final code freeze next week:

We understand that it is too late for 3.4.0, but we are trying to get the
changes in for 3.4.1.

> I see no bug reports in
> bugzilla relating to the crunch engine and these bugs have existed since
> the code was contributed; so this can't really be considered as fixing
> regressions even if these are bugs.

Well, if not having entries about Crunch related bugs in bugzilla is
something that will prevent their fixing, we can go ahead and do whatever
is needed. Are there any special (gcc) requirements we have to comply
with?

> It might be possible to consider this
> for 3.4.1; it will depend on how complex the patch ends up being.

We've tried to keep patch noise low. The patch contains the minimal
possible changes as per the discussion a few weeks ago. Only critical
issues have been resolved, no fancy stuff like flags/variables
removal/renaming, etc.

> However, my suggestion would be that you work on this for 3.5 (ie on the
> trunk).  The correct way to get changes into the compiler is to produce a
> version for the trunk and then back-port the change for a branch if
> required.

We are going to revise the Crunch related code in 3.5 even deeper. However
since it will be a while before this version is going to be stable enough
for production, Cirrus prefer to have known stable compiler - 3.4.x.

The goal for the near future is to get enough Crunch related fixes in 3.4.x
that will allow users to compile correct Linux Crunch powered apps.  After
that we'll focus on the mainline where significantly more work is
pending.

> ! /* Nonzero if this chip supports Cirrus Crunch coprocessor.  */
>   int arm_is_cirrus = 0;
>
> arm_is_cirrus needs to go.  It should be replaced with arm_arch_cirrus and
> arm_tune_cirrus as the context demands.

This code is a legacy from the previous contributor(s). Since the patch
should be as small possible we didn't change anything else but only the
strictly necessary bits.

> +       /* Cirrus Crunch coprocessor still requires soft-float division.
> */
>         arm_fpu_tune = FPUTYPE_MAVERICK;
> !       target_flags |= ARM_FLAG_SOFT_FLOAT;
>
> I think this is wrong too.   If you don't have a floating-point division
> insn, then just disable the divsf3 and divdf3 define_expand patterns for
> this target.  The compiler will then automatically call library functions
> for these cases.

This change just makes source code more readable - the effect is still the
same: setting the SOFT_FLOAT bit in the flags. And no, disabling "divsf3"
and "divdf3" patterns didn't help - it causes GCC to choke with the
following:

  error: unable to find a register to spill in class `FPA_REGS'

It is either me doing the pattern-disabling in a wrong way, or there's
relying somewhere in Crunch support on the SOFT_FLOAT bit set. Perhaps
Aldy could help?

> +   if (TARGET_CIRRUS)
> +     for (regno = FIRST_CIRRUS_FP_REGNUM; regno <= LAST_CIRRUS_FP_REGNUM;
> regno++)
> +       if (regs_ever_live[regno] && !call_used_regs[regno])
> + 	return 0;
>
> Another inconsistency -- how is TARGET_CIRRUS different from arm_is_cirrus?

What do you mean?

#define TARGET_CIRRUS (arm_is_cirrus)

Apart from initializing "arm_is_cirrus" variable and checking it once, all
the code refers to the macro "TARGET_CIRRUS", pretty much as everything
else is done. What is the inconsistency you saw?

> ! 	case LE: return (TARGET_CIRRUS) ? ARM_LE : ARM_LS;
> ! 	case LT: return (TARGET_CIRRUS) ? ARM_LT : ARM_MI;
>   	case NE: return ARM_NE;
>   	case EQ: return ARM_EQ;
>   	case ORDERED: return ARM_VC;
>   	case UNORDERED: return ARM_VS;
>   	case UNLT: return ARM_LT;
>   	case UNLE: return ARM_LE;
> ! 	case UNGT: return (TARGET_CIRRUS) ? ARM_GT : ARM_HI;
> ! 	case UNGE: return (TARGET_CIRRUS) ? ARM_GE : ARM_PL;
>
>
> I think this is also wrong.  How are the comparison bits set for an
> unordered result?  If nothing else, there must be a comment here
> describing why the code is different.

You're a bit right about this one. Crunch coprocessor does comparison
quite different from FPA/VFP:

         N  Z  C  V
A == B   0  1  0  0
A < B    1  0  0  0
A > B    1  0  0  1
unord.   0  0  0  0

As you can see, apart from EQ, NE, LT, LE, GT and GE, the others can't be
implemented in a single conditional ARM instruction.

What can you suggest as best solution? Do you think the following code
could break anything:

	case ORDERED: /* Fall through.  */
	case UNORDERED: /* Fall through.  */
	default: abort ();

> ! @item -mfix-crunch-d0
> ! @opindex mfix-crunch-d0
> ! @opindex mfix-crunch-d1
> ! Enable workarounds for the Cirrus MaverickCrunch coprocessor revisions
> ! D0 and D1.
>
> These might make some sense at the moment, but I see Cirrus have announced
> another 11 (?) new maverick chips recently.  So the question will soon be
> "Revision D<n> of which chip?"  I think you need to come up with a better
> name for this.

This is particularly true, also. Even the architecture/cpu "ep9312" is not
proper, since it is just arm920t with Crunch coprocessor, and other
maverick chips of the ep93xx family are pretty much the same to GCC. So
with the inclusion of the "-mfpu=..." option in 3.5 this "ep9312" target
could be eliminated or made obsolete.

As to the Crunch revisions, there are D0 and D1 currently, and perhaps
there will be E0 also.

> I also don't think we can really spare 2 bits of the main
> TARGET_FLAGS word for this;

Once again, for 3.4.x this seems harmless enough. For the mainline it
should be done the proper way. However, if that is totally unacceptable we'll
move Crunch related flags into another flag word.

Best regards,
  -- vladitx


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]