Re: [PATCH V3] Use preferred mode for doloop IV [PR61837]
On 2021-07-27 23:40, Jeff Law wrote: On 7/27/2021 12:27 AM, Richard Biener wrote: On Fri, 23 Jul 2021, Jeff Law wrote: On 7/15/2021 4:08 AM, Jiufu Guo via Gcc-patches wrote: Refine code for V2 according to review comments: * Use if check instead assert, and refine assert * Use better RE check for test case, e.g. (?n)/(?p) * Use better wording for target.def Currently, doloop.xx variable is using the type as niter which may be shorter than word size. For some targets, it would be better to use word size type. For example, on 64bit system, to access 32bit value, subreg maybe used. Then using 64bit type maybe better for niter if it can be present in both 32bit and 64bit. This patch add target hook for querg perferred mode for doloop IV. And update mode accordingly. Bootstrap and regtest pass on powerpc64le, is this ok for trunk? BR. Jiufu gcc/ChangeLog: 2021-07-15 Jiufu Guo PR target/61837 * config/rs6000/rs6000.c (TARGET_PREFERRED_DOLOOP_MODE): New hook. (rs6000_preferred_doloop_mode): New hook. * doc/tm.texi: Regenerate. * doc/tm.texi.in: Add hook preferred_doloop_mode. * target.def (preferred_doloop_mode): New hook. * targhooks.c (default_preferred_doloop_mode): New hook. * targhooks.h (default_preferred_doloop_mode): New hook. * tree-ssa-loop-ivopts.c (compute_doloop_base_on_mode): New function. (add_iv_candidate_for_doloop): Call targetm.preferred_doloop_mode and compute_doloop_base_on_mode. gcc/testsuite/ChangeLog: 2021-07-15 Jiufu Guo PR target/61837 * gcc.target/powerpc/pr61837.c: New test. My first reaction was that whatever type corresponds to the target's word_mode would be the right choice. But then I remembered things like dbCC on m68k which had a more limited range. While I don't think m68k uses the doloop bits, it's a clear example that the most desirable type may not correspond to the word type for the target. So my concern with this patch is its introducing more target dependencies into the gimple pipeline which is generally considered undesirable from a design standpoint. Is there any way to lower from whatever type is chosen by ivopts to the target's desired type at the gimple->rtl border rather than doing it in ivopts? I think that's difficult - after all we want to base other IV uses on the doloop IV if possible. So IMHO it's not different from IVOPTs choosing different IVs based on RTL costing and target addressing mode availability so I wasn't worried about those additional target dependences at this point of the GIMPLE pipeline. Yea, you're probably right on both accounts. With that resolved I think this is OK for the trunk. Thanks for your patience Jiufu and thanks for chiming in Richi. Thanks for all your help! The patch was committed to r12-2585. I notice that I ignored one guality case(gfortran.dg/guality/arg1.f90). It becomes 'unsupported' from 'pass'. The issue could be reproduced on a similar test case without this patch. Just opened PR101669 for it. BR, Jiufu jeff
Re: [PATCH V3] Use preferred mode for doloop IV [PR61837]
On 7/27/2021 12:27 AM, Richard Biener wrote: On Fri, 23 Jul 2021, Jeff Law wrote: On 7/15/2021 4:08 AM, Jiufu Guo via Gcc-patches wrote: Refine code for V2 according to review comments: * Use if check instead assert, and refine assert * Use better RE check for test case, e.g. (?n)/(?p) * Use better wording for target.def Currently, doloop.xx variable is using the type as niter which may be shorter than word size. For some targets, it would be better to use word size type. For example, on 64bit system, to access 32bit value, subreg maybe used. Then using 64bit type maybe better for niter if it can be present in both 32bit and 64bit. This patch add target hook for querg perferred mode for doloop IV. And update mode accordingly. Bootstrap and regtest pass on powerpc64le, is this ok for trunk? BR. Jiufu gcc/ChangeLog: 2021-07-15 Jiufu Guo PR target/61837 * config/rs6000/rs6000.c (TARGET_PREFERRED_DOLOOP_MODE): New hook. (rs6000_preferred_doloop_mode): New hook. * doc/tm.texi: Regenerate. * doc/tm.texi.in: Add hook preferred_doloop_mode. * target.def (preferred_doloop_mode): New hook. * targhooks.c (default_preferred_doloop_mode): New hook. * targhooks.h (default_preferred_doloop_mode): New hook. * tree-ssa-loop-ivopts.c (compute_doloop_base_on_mode): New function. (add_iv_candidate_for_doloop): Call targetm.preferred_doloop_mode and compute_doloop_base_on_mode. gcc/testsuite/ChangeLog: 2021-07-15 Jiufu Guo PR target/61837 * gcc.target/powerpc/pr61837.c: New test. My first reaction was that whatever type corresponds to the target's word_mode would be the right choice. But then I remembered things like dbCC on m68k which had a more limited range. While I don't think m68k uses the doloop bits, it's a clear example that the most desirable type may not correspond to the word type for the target. So my concern with this patch is its introducing more target dependencies into the gimple pipeline which is generally considered undesirable from a design standpoint. Is there any way to lower from whatever type is chosen by ivopts to the target's desired type at the gimple->rtl border rather than doing it in ivopts? I think that's difficult - after all we want to base other IV uses on the doloop IV if possible. So IMHO it's not different from IVOPTs choosing different IVs based on RTL costing and target addressing mode availability so I wasn't worried about those additional target dependences at this point of the GIMPLE pipeline. Yea, you're probably right on both accounts. With that resolved I think this is OK for the trunk. Thanks for your patience Jiufu and thanks for chiming in Richi. jeff
Re: [PATCH V3] Use preferred mode for doloop IV [PR61837]
On Fri, 23 Jul 2021, Jeff Law wrote: > > > On 7/15/2021 4:08 AM, Jiufu Guo via Gcc-patches wrote: > > Refine code for V2 according to review comments: > > * Use if check instead assert, and refine assert > > * Use better RE check for test case, e.g. (?n)/(?p) > > * Use better wording for target.def > > > > Currently, doloop.xx variable is using the type as niter which may be > > shorter than word size. For some targets, it would be better to use > > word size type. For example, on 64bit system, to access 32bit value, > > subreg maybe used. Then using 64bit type maybe better for niter if > > it can be present in both 32bit and 64bit. > > > > This patch add target hook for querg perferred mode for doloop IV. > > And update mode accordingly. > > > > Bootstrap and regtest pass on powerpc64le, is this ok for trunk? > > > > BR. > > Jiufu > > > > gcc/ChangeLog: > > > > 2021-07-15 Jiufu Guo > > > > PR target/61837 > > * config/rs6000/rs6000.c (TARGET_PREFERRED_DOLOOP_MODE): New hook. > > (rs6000_preferred_doloop_mode): New hook. > > * doc/tm.texi: Regenerate. > > * doc/tm.texi.in: Add hook preferred_doloop_mode. > > * target.def (preferred_doloop_mode): New hook. > > * targhooks.c (default_preferred_doloop_mode): New hook. > > * targhooks.h (default_preferred_doloop_mode): New hook. > > * tree-ssa-loop-ivopts.c (compute_doloop_base_on_mode): New function. > > (add_iv_candidate_for_doloop): Call targetm.preferred_doloop_mode > > and compute_doloop_base_on_mode. > > > > gcc/testsuite/ChangeLog: > > > > 2021-07-15 Jiufu Guo > > > > PR target/61837 > > * gcc.target/powerpc/pr61837.c: New test. > My first reaction was that whatever type corresponds to the target's word_mode > would be the right choice. But then I remembered things like dbCC on m68k > which had a more limited range. While I don't think m68k uses the doloop > bits, it's a clear example that the most desirable type may not correspond to > the word type for the target. > > So my concern with this patch is its introducing more target dependencies into > the gimple pipeline which is generally considered undesirable from a design > standpoint. Is there any way to lower from whatever type is chosen by ivopts > to the target's desired type at the gimple->rtl border rather than doing it in > ivopts? I think that's difficult - after all we want to base other IV uses on the doloop IV if possible. So IMHO it's not different from IVOPTs choosing different IVs based on RTL costing and target addressing mode availability so I wasn't worried about those additional target dependences at this point of the GIMPLE pipeline. Richard.
Re: [PATCH V3] Use preferred mode for doloop IV [PR61837]
Jeff Law writes: On 7/15/2021 4:08 AM, Jiufu Guo via Gcc-patches wrote: Refine code for V2 according to review comments: * Use if check instead assert, and refine assert * Use better RE check for test case, e.g. (?n)/(?p) * Use better wording for target.def Currently, doloop.xx variable is using the type as niter which may be shorter than word size. For some targets, it would be better to use word size type. For example, on 64bit system, to access 32bit value, subreg maybe used. Then using 64bit type maybe better for niter if it can be present in both 32bit and 64bit. This patch add target hook for querg perferred mode for doloop IV. And update mode accordingly. Bootstrap and regtest pass on powerpc64le, is this ok for trunk? BR. Jiufu gcc/ChangeLog: 2021-07-15 Jiufu Guo PR target/61837 * config/rs6000/rs6000.c (TARGET_PREFERRED_DOLOOP_MODE): New hook. (rs6000_preferred_doloop_mode): New hook. * doc/tm.texi: Regenerate. * doc/tm.texi.in: Add hook preferred_doloop_mode. * target.def (preferred_doloop_mode): New hook. * targhooks.c (default_preferred_doloop_mode): New hook. * targhooks.h (default_preferred_doloop_mode): New hook. * tree-ssa-loop-ivopts.c (compute_doloop_base_on_mode): New function. (add_iv_candidate_for_doloop): Call targetm.preferred_doloop_mode and compute_doloop_base_on_mode. gcc/testsuite/ChangeLog: 2021-07-15 Jiufu Guo PR target/61837 * gcc.target/powerpc/pr61837.c: New test. Hi Jeff, My first reaction was that whatever type corresponds to the target's word_mode would be the right choice. But then I remembered things like dbCC on m68k which had a more limited range. While I don't think m68k uses the doloop bits, it's a clear example that the most desirable type may not correspond to the word type for the target. I had a check about the implementation of doloop_end insn again. m68k seems do not implement this insn. On most of these targets who implement doloop_end insn, word_mode meets the requirement of doloop_end insn. s390/tilegx and pru are little different. s390 checks SI/DI and arch; tilegx is using if-then-else-jump; on pru, it seems weird: UNITS_PER_WORD is defined as 1(then word_mode 8bits), while doloop_end rejects QImode. So, word_mode would be suitable for most targets, while it may not be best choice for some targets as you said. Then this patch introducing a hook for target to tune. So my concern with this patch is its introducing more target dependencies into the gimple pipeline which is generally considered undesirable from a design standpoint. Is there any way to lower from whatever type is chosen by ivopts to the target's desired type at the gimple->rtl border rather than doing it in ivopts? Currently, once gimple choice the type of doloop iv, we keep it until rtl doloop pass and then some suboptimize (like subreg access) was living into asm code. To change the type during lower doloop iv (at expend pass?), we may also need a hook. Choosing suitable type early at gimple for doloop iv may avoid some sub-optimization(e.g. type conversion) for all following passes. So, this patch selects the preferred type for doloop iv when it was generated. Thanks for your review and further comments! BR, Jiufu jeff
Re: [PATCH V3] Use preferred mode for doloop IV [PR61837]
On 7/15/2021 4:08 AM, Jiufu Guo via Gcc-patches wrote: Refine code for V2 according to review comments: * Use if check instead assert, and refine assert * Use better RE check for test case, e.g. (?n)/(?p) * Use better wording for target.def Currently, doloop.xx variable is using the type as niter which may be shorter than word size. For some targets, it would be better to use word size type. For example, on 64bit system, to access 32bit value, subreg maybe used. Then using 64bit type maybe better for niter if it can be present in both 32bit and 64bit. This patch add target hook for querg perferred mode for doloop IV. And update mode accordingly. Bootstrap and regtest pass on powerpc64le, is this ok for trunk? BR. Jiufu gcc/ChangeLog: 2021-07-15 Jiufu Guo PR target/61837 * config/rs6000/rs6000.c (TARGET_PREFERRED_DOLOOP_MODE): New hook. (rs6000_preferred_doloop_mode): New hook. * doc/tm.texi: Regenerate. * doc/tm.texi.in: Add hook preferred_doloop_mode. * target.def (preferred_doloop_mode): New hook. * targhooks.c (default_preferred_doloop_mode): New hook. * targhooks.h (default_preferred_doloop_mode): New hook. * tree-ssa-loop-ivopts.c (compute_doloop_base_on_mode): New function. (add_iv_candidate_for_doloop): Call targetm.preferred_doloop_mode and compute_doloop_base_on_mode. gcc/testsuite/ChangeLog: 2021-07-15 Jiufu Guo PR target/61837 * gcc.target/powerpc/pr61837.c: New test. My first reaction was that whatever type corresponds to the target's word_mode would be the right choice. But then I remembered things like dbCC on m68k which had a more limited range. While I don't think m68k uses the doloop bits, it's a clear example that the most desirable type may not correspond to the word type for the target. So my concern with this patch is its introducing more target dependencies into the gimple pipeline which is generally considered undesirable from a design standpoint. Is there any way to lower from whatever type is chosen by ivopts to the target's desired type at the gimple->rtl border rather than doing it in ivopts? jeff
[PATCH V3] Use preferred mode for doloop IV [PR61837]
Refine code for V2 according to review comments:
* Use if check instead assert, and refine assert
* Use better RE check for test case, e.g. (?n)/(?p)
* Use better wording for target.def
Currently, doloop.xx variable is using the type as niter which may be
shorter than word size. For some targets, it would be better to use
word size type. For example, on 64bit system, to access 32bit value,
subreg maybe used. Then using 64bit type maybe better for niter if
it can be present in both 32bit and 64bit.
This patch add target hook for querg perferred mode for doloop IV.
And update mode accordingly.
Bootstrap and regtest pass on powerpc64le, is this ok for trunk?
BR.
Jiufu
gcc/ChangeLog:
2021-07-15 Jiufu Guo
PR target/61837
* config/rs6000/rs6000.c (TARGET_PREFERRED_DOLOOP_MODE): New hook.
(rs6000_preferred_doloop_mode): New hook.
* doc/tm.texi: Regenerate.
* doc/tm.texi.in: Add hook preferred_doloop_mode.
* target.def (preferred_doloop_mode): New hook.
* targhooks.c (default_preferred_doloop_mode): New hook.
* targhooks.h (default_preferred_doloop_mode): New hook.
* tree-ssa-loop-ivopts.c (compute_doloop_base_on_mode): New function.
(add_iv_candidate_for_doloop): Call targetm.preferred_doloop_mode
and compute_doloop_base_on_mode.
gcc/testsuite/ChangeLog:
2021-07-15 Jiufu Guo
PR target/61837
* gcc.target/powerpc/pr61837.c: New test.
---
gcc/config/rs6000/rs6000.c | 11
gcc/doc/tm.texi| 9 +++
gcc/doc/tm.texi.in | 2 +
gcc/target.def | 13
gcc/targhooks.c| 8 +++
gcc/targhooks.h| 1 +
gcc/testsuite/gcc.target/powerpc/pr61837.c | 20 +++
gcc/tree-ssa-loop-ivopts.c | 69 +-
8 files changed, 131 insertions(+), 2 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/powerpc/pr61837.c
diff --git a/gcc/config/rs6000/rs6000.c b/gcc/config/rs6000/rs6000.c
index 9a5db63d0ef..3bdf0cb97a3 100644
--- a/gcc/config/rs6000/rs6000.c
+++ b/gcc/config/rs6000/rs6000.c
@@ -1700,6 +1700,9 @@ static const struct attribute_spec
rs6000_attribute_table[] =
#undef TARGET_DOLOOP_COST_FOR_ADDRESS
#define TARGET_DOLOOP_COST_FOR_ADDRESS 10
+#undef TARGET_PREFERRED_DOLOOP_MODE
+#define TARGET_PREFERRED_DOLOOP_MODE rs6000_preferred_doloop_mode
+
#undef TARGET_ATOMIC_ASSIGN_EXPAND_FENV
#define TARGET_ATOMIC_ASSIGN_EXPAND_FENV rs6000_atomic_assign_expand_fenv
@@ -27867,6 +27870,14 @@ rs6000_predict_doloop_p (struct loop *loop)
return true;
}
+/* Implement TARGET_PREFERRED_DOLOOP_MODE. */
+
+static machine_mode
+rs6000_preferred_doloop_mode (machine_mode)
+{
+ return word_mode;
+}
+
/* Implement TARGET_CANNOT_SUBSTITUTE_MEM_EQUIV_P. */
static bool
diff --git a/gcc/doc/tm.texi b/gcc/doc/tm.texi
index 2a41ae5fba1..fcfebc2ae37 100644
--- a/gcc/doc/tm.texi
+++ b/gcc/doc/tm.texi
@@ -11984,6 +11984,15 @@ By default, the RTL loop optimizer does not use a
present doloop pattern for
loops containing function calls or branch on table instructions.
@end deftypefn
+@deftypefn {Target Hook} machine_mode TARGET_PREFERRED_DOLOOP_MODE
(machine_mode @var{mode})
+This hook takes a @var{mode} for a doloop IV, where @code{mode} is the
+original mode for the operation. If the target prefers an alternate
+@code{mode} for the operation, then this hook should return that mode;
+otherwise the original @code{mode} should be returned. For example, on a
+64-bit target, @code{DImode} might be preferred over @code{SImode}. Both the
+original and the returned modes should be @code{MODE_INT}.
+@end deftypefn
+
@deftypefn {Target Hook} bool TARGET_LEGITIMATE_COMBINED_INSN (rtx_insn
*@var{insn})
Take an instruction in @var{insn} and return @code{false} if the instruction
is not appropriate as a combination of two or more instructions. The
diff --git a/gcc/doc/tm.texi.in b/gcc/doc/tm.texi.in
index f881cdabe9e..38215149a92 100644
--- a/gcc/doc/tm.texi.in
+++ b/gcc/doc/tm.texi.in
@@ -7917,6 +7917,8 @@ to by @var{ce_info}.
@hook TARGET_INVALID_WITHIN_DOLOOP
+@hook TARGET_PREFERRED_DOLOOP_MODE
+
@hook TARGET_LEGITIMATE_COMBINED_INSN
@hook TARGET_CAN_FOLLOW_JUMP
diff --git a/gcc/target.def b/gcc/target.def
index c009671c583..892c97550b2 100644
--- a/gcc/target.def
+++ b/gcc/target.def
@@ -4454,6 +4454,19 @@ loops containing function calls or branch on table
instructions.",
const char *, (const rtx_insn *insn),
default_invalid_within_doloop)
+/* Returns the machine mode which the target prefers for doloop IV. */
+DEFHOOK
+(preferred_doloop_mode,
+"This hook takes a @var{mode} for a doloop IV, where @code{mode} is the\n\
+original mode for the operation. If the target prefers an alternate\n\
+@code{mode} for the operation, then this hook should return that mode;\n\
+otherwise the original
