, V7, #3 of 7, Use PADDI for 34-bit immediate adds
diff mbox series

Message ID 20191114224442.GC7528@ibm-toto.the-meissners.org
State New
Headers show
Series
  • , V7, #3 of 7, Use PADDI for 34-bit immediate adds
Related show

Commit Message

Michael Meissner Nov. 14, 2019, 10:44 p.m. UTC
This patch generates PADDI to add 34-bit immediate constants on the 'future'
system, and prevents such adds from being split.

I have built and boostrapped compilers with the patch, and there were no
regressions.  Can I check this into the trunk?

2019-11-14  Michael Meissner  <meissner@linux.ibm.com>

	* config/rs6000/predicates.md (add_operand): Add support for
	PADDI.
	* config/rs6000/rs6000.md (add<mode>3): Add support for PADDI.

Comments

Segher Boessenkool Nov. 23, 2019, 12:32 a.m. UTC | #1
On Thu, Nov 14, 2019 at 05:44:42PM -0500, Michael Meissner wrote:
> This patch generates PADDI to add 34-bit immediate constants on the 'future'
> system, and prevents such adds from being split.

I don't see that last part?  Is that a remnant of a previous version of
the patch?

> 	* config/rs6000/predicates.md (add_operand): Add support for
> 	PADDI.

More directly: Allow constants that satisfy eI.

> 	* config/rs6000/rs6000.md (add<mode>3): Add support for PADDI.

(add<mode>3 for GPR): etc.

We have six things all called add<mode>3 (VI2, DDTD, SDI, SFDF, IEEE128,
VEC_F).  Erm.

Wait, this is called *add<mode>3, not add<mode>3.


Okay for trunk with that fixed.  Thanks,


Segher
Michael Meissner Nov. 25, 2019, 10:40 p.m. UTC | #2
On Fri, Nov 22, 2019 at 06:32:08PM -0600, Segher Boessenkool wrote:
> On Thu, Nov 14, 2019 at 05:44:42PM -0500, Michael Meissner wrote:
> > This patch generates PADDI to add 34-bit immediate constants on the 'future'
> > system, and prevents such adds from being split.
> 
> I don't see that last part?  Is that a remnant of a previous version of
> the patch?

This falls out of the add_operand change.

The add splitter is:

;; Split an add that we can't do in one insn into two insns, each of which
;; does one 16-bit part.  This is used by combine.  Note that the low-order
;; add should be last in case the result gets used in an address.

(define_split
  [(set (match_operand:GPR 0 "gpc_reg_operand")
	(plus:GPR (match_operand:GPR 1 "gpc_reg_operand")
		  (match_operand:GPR 2 "non_add_cint_operand")))]
  ""
  [(set (match_dup 0) (plus:GPR (match_dup 1) (match_dup 3)))
   (set (match_dup 0) (plus:GPR (match_dup 0) (match_dup 4)))]
{
  HOST_WIDE_INT val = INTVAL (operands[2]);
  HOST_WIDE_INT low = ((val & 0xffff) ^ 0x8000) - 0x8000;
  HOST_WIDE_INT rest = trunc_int_for_mode (val - low, <MODE>mode);

  operands[4] = GEN_INT (low);
  if (<MODE>mode == SImode || satisfies_constraint_L (GEN_INT (rest)))
    operands[3] = GEN_INT (rest);
  else if (can_create_pseudo_p ())
    {
      operands[3] = gen_reg_rtx (DImode);
      emit_move_insn (operands[3], operands[2]);
      emit_insn (gen_adddi3 (operands[0], operands[1], operands[3]));
      DONE;
    }
  else
    FAIL;
})

Since non_add_cint_operand is defined as:

(define_predicate "non_add_cint_operand"
  (and (match_code "const_int")
       (not (match_operand 0 "add_operand"))))

It follows that if add_operand now allows a 34-bit constant when -mcpu=future,
that non_add_cint_operand will return 0, and the add will not be split.

> > 	* config/rs6000/predicates.md (add_operand): Add support for
> > 	PADDI.
> 
> More directly: Allow constants that satisfy eI.

Ok.

> > 	* config/rs6000/rs6000.md (add<mode>3): Add support for PADDI.
> 
> (add<mode>3 for GPR): etc.
> 
> We have six things all called add<mode>3 (VI2, DDTD, SDI, SFDF, IEEE128,
> VEC_F).  Erm.
> 
> Wait, this is called *add<mode>3, not add<mode>3.

Yeah, I usually add the iterator part to identify which one was changed.
> 
> Okay for trunk with that fixed.  Thanks,
> 
> 
> Segher
Segher Boessenkool Nov. 26, 2019, 12:55 a.m. UTC | #3
On Mon, Nov 25, 2019 at 05:40:02PM -0500, Michael Meissner wrote:
> On Fri, Nov 22, 2019 at 06:32:08PM -0600, Segher Boessenkool wrote:
> > On Thu, Nov 14, 2019 at 05:44:42PM -0500, Michael Meissner wrote:
> > > This patch generates PADDI to add 34-bit immediate constants on the 'future'
> > > system, and prevents such adds from being split.
> > 
> > I don't see that last part?  Is that a remnant of a previous version of
> > the patch?
> 
> This falls out of the add_operand change.

<snip>

> Since non_add_cint_operand is defined as:
> 
> (define_predicate "non_add_cint_operand"
>   (and (match_code "const_int")
>        (not (match_operand 0 "add_operand"))))
> 
> It follows that if add_operand now allows a 34-bit constant when -mcpu=future,
> that non_add_cint_operand will return 0, and the add will not be split.

Right, so write that please ("changes add_operand"), not some random
consequence of that change.

> > > 	* config/rs6000/rs6000.md (add<mode>3): Add support for PADDI.
> > 
> > (add<mode>3 for GPR): etc.
> > 
> > We have six things all called add<mode>3 (VI2, DDTD, SDI, SFDF, IEEE128,
> > VEC_F).  Erm.
> > 
> > Wait, this is called *add<mode>3, not add<mode>3.
> 
> Yeah, I usually add the iterator part to identify which one was changed.

No, the asterisk is part of the name, that's my point.  And yes please
say what the iterator is.


Segher

Patch
diff mbox series

Index: gcc/config/rs6000/predicates.md
===================================================================
--- gcc/config/rs6000/predicates.md	(revision 278173)
+++ gcc/config/rs6000/predicates.md	(working copy)
@@ -839,7 +839,8 @@  (define_special_predicate "indexed_addre
 (define_predicate "add_operand"
   (if_then_else (match_code "const_int")
     (match_test "satisfies_constraint_I (op)
-		 || satisfies_constraint_L (op)")
+		 || satisfies_constraint_L (op)
+		 || satisfies_constraint_eI (op)")
     (match_operand 0 "gpc_reg_operand")))
 
 ;; Return 1 if the operand is either a non-special register, or 0, or -1.
Index: gcc/config/rs6000/rs6000.md
===================================================================
--- gcc/config/rs6000/rs6000.md	(revision 278176)
+++ gcc/config/rs6000/rs6000.md	(working copy)
@@ -1761,15 +1761,17 @@  (define_expand "add<mode>3"
 })
 
 (define_insn "*add<mode>3"
-  [(set (match_operand:GPR 0 "gpc_reg_operand" "=r,r,r")
-	(plus:GPR (match_operand:GPR 1 "gpc_reg_operand" "%r,b,b")
-		  (match_operand:GPR 2 "add_operand" "r,I,L")))]
+  [(set (match_operand:GPR 0 "gpc_reg_operand" "=r,r,r,r")
+	(plus:GPR (match_operand:GPR 1 "gpc_reg_operand" "%r,b,b,b")
+		  (match_operand:GPR 2 "add_operand" "r,I,L,eI")))]
   ""
   "@
    add %0,%1,%2
    addi %0,%1,%2
-   addis %0,%1,%v2"
-  [(set_attr "type" "add")])
+   addis %0,%1,%v2
+   addi %0,%1,%2"
+  [(set_attr "type" "add")
+   (set_attr "isa" "*,*,*,fut")])
 
 (define_insn "*addsi3_high"
   [(set (match_operand:SI 0 "gpc_reg_operand" "=b")