RE: [Patch 3/5] rs6000, Add TI to TD (128-bit DFP) and TD to TI support

[Carl Love <cel@us.ibm.com>]

Segher:

On Wed, 2020-08-19 at 20:29 -0500, Segher Boessenkool wrote:
> Hi!
> 
> On Tue, Aug 11, 2020 at 12:22:59PM -0700, Carl Love wrote:
> > +(define_insn "floattitd2"
> > +  [(set (match_operand:TD 0 "gpc_reg_operand" "=d")
> > +	(float:TD (match_operand:TI 1 "gpc_reg_operand" "v")))]
> > +  "TARGET_TI_VECTOR_OPS"
> > +  "dcffixqq %0,%1"
> > +  [(set_attr "type" "dfp")])
> 
> I wonder if this should just be TARGET_POWER10 now?  That goes for
> the
> whole series of course.
> 
<snip>
> 
> Looks fine otherwise.  Okay for trunk, modulo whatever we do with
> YARGET_TI_VECTOR_OPS.  Thanks!
> 
> 
> Segher

You commented on the TARGET_TI_VECTOR_OPS in patch 2 as well, i.e.

   > > +  /* The -mti-vector-ops option requires ISA 3.1 support and -maltivec for
   > > +     the 128-bit instructions.  Currently, TARGET_POWER10 is sufficient to
   > > +     enable it by default.  */
   > > +  if (TARGET_POWER10)
   > > +    {
   > > +      if (rs6000_isa_flags_explicit & OPTION_MASK_VSX)
   > > +   warning(0, ("%<-mno-altivec%> disables -mti-vector-ops (128-bit integer vector register operations)."));
   > > +      else
   > > +   rs6000_isa_flags |= OPTION_MASK_TI_VECTOR_OPS;
   > > +    }
   > 
   > It seems odd here that -maltivec is explicitly called out here.  That
   > should be default on for quite a while at this point.

   And the actual check it for -mvsx anyway?  Not sure I follow what this
   does at all.

So this all goes way back, the following is from a discussion way back
in November.   We added it thinking it would be good to be able to
enable/disable the 128-bit vector support, as I recall.  I have to
admit that I am struggling a bit to remember all the details as we
discussed them back then. 

The following is from an old email.


   Hi Carl,

   On Mon, Nov 18, 2019 at 12:18:48PM -0800, Carl Love wrote:
   > Per your other note, I change the test suite check, with spelling fix,
   > to:
   >  
   > # Return 1 if the can generate the 128-bit integer operations in ISA 3.1.
   > # That means the following 128-bit instructions can be generated:
   > # vadduqm, vsubuqm, vmsumcud, vdivsq, vdivuq, vmodsq, vmoduq, vcmpuq, vcmpsq,
   > # vrlq, vslq, vsrq.  The -mti-vector-ops flag enables the needed support.
   > # The -mti-vector-ops is enabled by default for -mcpu=future.  Sufficient
   > # to test if the vadduqm instruction is generated for ISA 3.1 support.
   > proc check_effective_target_ppc_native_128bit { } {                             
   >     return [check_no_messages_and_pattern_nocache \
   >                 ppc_native_128bit {\mvadduqm\M} assembly {
   >                     __int128_t test_add (__int128_t a, __int128_t b)
   >                     { return a + b; }           
   >                 } {-mti-vector-ops} ]
   > }

   Ah, very good, this will work fine :-)

   But without that -mti-vector-ops option?

   > > (We also need to test what happens with -mno-altivec, etc. --
   > > shouldn't
   > > be hard, should just do the same thing as it does on older CPUs).
   > 
   > So I tried compiling the test case with -mn-altivec and -mcpu=future
   > and I get a GCC crash.  :-(

   Think of it this way: it is good if developers get ICEs.  That way, users
   get fewer.  :-)

   > I updated the setting for rs6000_isa_flags in rs6000.c to:
   > 
   >   /* The -mti-vector-ops option requires ISA 3.1 support and -maltivec for
   >      the 128-bit instructions.  Currently, TARGET_FUTURE is sufficient to
   >      enable it by default.  */                                 
   >   if (TARGET_FUTURE)                                                            
   >     {                                                                           
   >       if (rs6000_isa_flags_explicit & OPTION_MASK_VSX)
   >         warning(0, ("%<-mno-altivec%> disables -mti-vector-ops (128-bit integer vector register operations)."));                            
   >       else                                                                      
   >         rs6000_isa_flags |= OPTION_MASK_TI_VECTOR_OPS;                          
   >     }
   >  
   > Now, I get:
   >  
   > $GCC_INSTALL/bin/gcc -g -mcpu=future -mno-altivec  int_128bit-runnable.c -o int_128bit-runnable
   > cc1: warning: ‘-mno-altivec’ disables vsx
   > cc1: warning: ‘-mno-altivec’ disables -mti-vector-ops (128-bit integer vector register operations).
   > 
   > without -mno-altivec it compiles with no warning or errors.  The object
   > dump has the expected vadduqm and vsubuqm instructions.

   That will do the trick.  Maybe you want the message to be quiet, (or
   quieter anyway), if you get testsuite fallout?  You will find out.

   > > > +mti-vector-ops
   > > > +Target Report Mask(TI_VECTOR_OPS) Var(rs6000_isa_flags)
   > > > +Use integer 128-bit instructions for a future architecture.
   > > 
   > > For upstream we should make this an internal option (so
   > > Undocumented), but
   > > it may be handy to keep the option more visible for now, sure.
   > 
   > I was kinda thinking it would probably be made internal down the road.
   > 
   > So with these changes, I think we have a good base patch to build on. 

   Yes!  Thank you, this looks like it will be much less problematic than I
   feared for :-)


   Segher

So, do we want to drop the option OPTION_MASK_TI_VECTOR_OPS at this
point and go with just TARGET_POWER10?  
 

                        Carl