This is the mail archive of the 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)

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

That will have to depend on what policy Mark sets for the 3.4 branch after 
3.4.0 is released.  If he opens it up for all bugs, then yes, this may 
well qualify.  On the other hand, if he only allows regression fixes then 
no, this code will not qualify since there has never been a release of gcc 
where the problems did not exist.

> > 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?

Bug reports are useful, particularly if they have test cases which can 
subsequently be made into regression tests.  However, they are most 
important if the problem is a regression.

> 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.

Don't worry about this one then, given the context of the patch it is not 

> > +       /* 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?

Ah! This is because TARGET_HARD_FLOAT is defined as !TARGET_SOFT_FLOAT in 
the 3.4 branch sources -- that's fixed on the trunk, where the patterns 
are more clearly separated (TARGET_HARD_FLOAT will now be any of FPA, VFP 
and CRUNCH; other flags will determine which).

OK, I'll let this one pass for the moment.

> > +   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?

I think I'd prefer that TARGET_CIRRUS were removed entirely and the 
underlying definition used everywhere.  Again, on reflection this is not 
critical given the context of what you are doing here.

> > ! 	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 ();

No, this really is all wrong.  Remember that in IEEE FP predicates the 
opposite of LT is UNGE (not GE) etc.  The only exception to this in GCC's 
implementation is that NE is still the opposite of EQ (that is, that NE is 
always treated as the IEEE ?<> predicate).  It can't be right therefore to 
have GT and UNGT return the same codes.

Looking at the above table suggests that you need to make a GT comparison 
use ARM_VS (hence UNLE should use ARM_VC), LT should use ARM_LT (UNGE -> 
ARM_GE), LE should use ARM_LE (UNGT -> ARM_GT), and GE is impossible in 
one test (needs to test V | Z), hence UNLT is also impossible.  It's also 
not possible to test for ORDERED or UNORDERED in a single test, but those 
aren't very important.  You can, however, do UNEQ and LTGT (which can't 
normally be done on the ARM) with ARM_PL and ARM_MI respectively.

You will need to adjust the b<cond> and s<cond> expanders/patterns to 
account for all this.

Rather than putting a load of ?: operators in the existing text, please 
use a separate switch table when compiling for Maverick.

> > ! @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.

In the light of this discussion, the issue here is more to do with the 
names of the options than how they are managed in the compiler.  It 
becomes difficult for the user if flags change name too often (appearing 
in one release and changing in the next).  Please give careful 
consideration to what naming scheme you will be using in future releases 
of the compiler, and consider whether these can also be used in this 
release.  Nobody has 20-20 foresight, but when we do know the likely 
future direction we should take account of it and avoid creating options 
that we know will soon be obsolete.


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