Hi Richard,
Thanks for your comments. I am working on the review comments, and will share
the reworked patch soon.
However, here is clarification on some of the issues raised.
> > + if (TARGET_FIX_24K && TUNE_P5600)
> > + error ("unsupported combination: %s", "-mtune=p5600 -mfix-24k");
> > +
> > /* Save the base compression state and process flags as though we
> > were generating uncompressed code. */
> > mips_base_compression_flags = TARGET_COMPRESSION;
>
> Although it's a bit of an odd combination, we need to accept -mfix-24k -
> mtune=p5600 and continue to implement the 24k workarounds.
> The idea is that a distributor can build for a common base architecture, add -
> mfix- options for processors that might run the code, and add -mtune= for
> the processor that's most of interest optimisation-wise.
>
> We should just make the pairing of stores conditional on !TARGET_FIX_24K.
We had offline discussion based on your comment. There is additional view on
the same.
Only ISAs mips32r2, mips32r3 and mips32r5 support P5600. Remaining ISAs do not
support P5600.
For mips32r2 (24K) and mips32r3 (micromips), load-store pairing is implemented
separately, and hence, as you suggested, P5600 Ld-ST bonding optimization
should not be enabled for them.
So, is it fine if I emit error for any ISAs other than mips32r2, mips32r3 and
mips32r5 when P5600 is enabled, or the compilation should continue by emitting
warning and disabling P5600?
Also, the optimization will be enabled only if !TARGET_FIX_24K &&
!TARGET_MICTOMIPS as suggested by you.
> > +
> > +#define ENABLE_LD_ST_PAIRING \
> > + (TARGET_ENABLE_LD_ST_PAIRING && TUNE_P5600)
>
> The patch requires -mld-st-pairing to be passed explicitly even for -
> mtune=p5600. Is that because it's not a consistent enough win for us to
> enable it by default? It sounded from the description like it should be an
> improvement more often that not.
>
> We should allow pairing even without -mtune=p5600.
Performance testing for this patch is not yet done.
If the patch proves beneficial in most of the testcases (which we believe will
do on P5600) we will enable this optimization by default for P5600 - in which
case this option can be removed.
>
> Are QImodes not paired in the same way? If so, it'd be worth adding a
> comment above the define_mode_iterator saying that QI is deliberately
> excluded.
The P5600 datasheet mentions bonding of load/stores in HI, SI, SF and DF modes
only. Hence QI mode is excluded. I will add the comment on the iterator.
- Thanks and regards,
Sameera D.
