On Tue, Jul 21, 2015 at 01:14:44PM +0200, Ulrich Weigand wrote:
> Dominik Vogt wrote:
> This version is looking good, except for one problem:
> 
> > -  "%{!m31:%{!m64:-m64}}",					\
> > -  "%{!mesa:%{!mzarch:%{m31:-mesa}%{m64:-mzarch}}}",		\
> > -  "%{!march=*:%{mesa:-march=g5}%{mzarch:-march=z900}}",		\
> 
> There's a reason for this particular sequence.  The first line ensures
> that one of -m64 or -m31 is present.  The second line ensures that one
> of -mesa or -mzarch is present, but this works only if already one of
> -m64 or -m31 is present, so it needs to come *after* the first line.
> The third line ensures that some -march= switch is present, but this
> works only if already one of -mesa or -mzarch is present, so it needs
> to comer *after* the second line.
> 
> > +#define DRIVER_SELF_SPECS					\
> > +  "%{!m31:%{!m64:-m" S390_TARGET_BITS_STRING "}} "		\
> > +  "%{!march=*:%{mesa:-march=g5}%{mzarch:-march=z900}} "		\
> > +  MARCH_MTUNE_NATIVE_SPECS					\
> > +  "%{!mesa:%{!mzarch:%{m31:-mesa}%{m64:-mzarch}}} "
> 
> This inverts the order of the second and third lines, so it is now
> no longer guaranteed that at least one -march= switch is present.
> 
> I understand that you need to move MARCH_MTUNE_NATIVE_SPECS ahead
> of the -mesa/-mzarch defaulting rule, but it should be possible
> to do that without changing the sequence of the three existing
> rules.  Why not just move MARCH_MTUNE_NATIVE_SPECS first?

Okay, good point.  Since this is a bit complicated, let's consider
which effect this change would have on the following two cases in
conjunction with -march=native:

1. local_cpu_detect returns -march=<something>

   Before suggested change: -march=<something> takes effect
   After change: -march=<something> takes effect

   => Correct behaviour in both cases, the second line never does
   anything.

2. local_cpu_detect returns nothing (empty string)

   Before change: No architecture string is used at all -> Bad!
   After change: The default defined by the second line kicks in.

   So the current code has even another bug, apart from the issue
   you have mentioned.

Given the above line numbering (1, 2, 3, 4), the order of
activation of the spec rules should be

 * 1, 4        with -march=<something> (<something> != native)
 * 1, 4, 2     without -march=<something>
 * 1, 3, 4     with -march=native (if local_cpu_detect returns a string)
 * 1, 3, 4, 2  with -march=native (if local_cpu_detect returns nothing)

So, the suggested order 1, 3, 4, 2 should cover all these cases.

Also, there's another bug.  The spec lines need to be separated by
commas.  This change is also contained in the attached new patch
(v4).

Ciao

Dominik ^_^  ^_^

-- 

Dominik Vogt
IBM Germany