| 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 | expand |
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
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
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
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")