| Message ID | 568D7CBF.2070407@samsung.com |
|---|---|
| State | New |
| Headers | show |
On 01/06/16 14:44, Evandro Menezes wrote: > Hi, Wilco. > > On 01/06/2016 06:04 AM, Wilco Dijkstra wrote: >>> Here's what I had in mind when I inquired about distinguishing FCMP >>> from >>> FCCMP. As you can see in the patch, Exynos is the only target that >>> cares about it, but I wonder if ThunderX or Xgene would too. >>> >>> What do you think? >> The new attributes look fine (I've got a similar outstanding change), >> however >> please don't add them to non-AArch64 cores. We only need it for >> thunderx.md, >> cortex-a53.md, cortex-a57.md, xgene1.md and exynos-m1.md. > > Add support for the FCCMP insn types > > 2016-01-04 Evandro Menezes <e.menezes@samsung.com> > > gcc/ > * config/aarch64/aarch64.md (fccmp): Change insn type. > (fccmpe): Likewise. > * config/aarch64/thunderx.md (thunderx_fcmp): Add > "fccmp{s,d}" types. > * config/arm/cortex-a53.md (cortex_a53_fpalu): Likewise. > * config/arm/cortex-a57.md (cortex_a57_fp_cmp): Likewise. > * config/arm/xgene1.md (xgene1_fcmp): Likewise. > * config/arm/exynos-m1.md (exynos_m1_fp_ccmp): New insn > reservation. > * config/arm/types.md (fccmps): Add new insn type. > (fccmpd): Likewise. > > Got it. Here's an updated patch. Again, assuming that your original > patch is in place. Perhaps you can build on it. > > Thank you, > Ping.
On Wed, Jan 06, 2016 at 02:44:47PM -0600, Evandro Menezes wrote: > Hi, Wilco. > > On 01/06/2016 06:04 AM, Wilco Dijkstra wrote: > >>Here's what I had in mind when I inquired about distinguishing FCMP from > >>FCCMP. As you can see in the patch, Exynos is the only target that > >>cares about it, but I wonder if ThunderX or Xgene would too. > >> > >>What do you think? > >The new attributes look fine (I've got a similar outstanding change), however > >please don't add them to non-AArch64 cores. We only need it for thunderx.md, > >cortex-a53.md, cortex-a57.md, xgene1.md and exynos-m1.md. > > Add support for the FCCMP insn types > > 2016-01-04 Evandro Menezes <e.menezes@samsung.com> > > gcc/ > * config/aarch64/aarch64.md (fccmp): Change insn type. > (fccmpe): Likewise. > * config/aarch64/thunderx.md (thunderx_fcmp): Add > "fccmp{s,d}" types. > * config/arm/cortex-a53.md (cortex_a53_fpalu): Likewise. > * config/arm/cortex-a57.md (cortex_a57_fp_cmp): Likewise. > * config/arm/xgene1.md (xgene1_fcmp): Likewise. > * config/arm/exynos-m1.md (exynos_m1_fp_ccmp): New insn > reservation. > * config/arm/types.md (fccmps): Add new insn type. > (fccmpd): Likewise. > > Got it. Here's an updated patch. Again, assuming that your > original patch is in place. Perhaps you can build on it. If we don't have any targets which care about the fccmps/fccmpd split in the code base, do we really need it? Can we just follow the example of fcsel? > diff --git a/gcc/config/arm/types.md b/gcc/config/arm/types.md > index 321ff89..daf7162 100644 > --- a/gcc/config/arm/types.md > +++ b/gcc/config/arm/types.md > @@ -70,6 +70,7 @@ > ; f_rint[d,s] double/single floating point rount to integral. > ; f_store[d,s] double/single store to memory. Used for VFP unit. > ; fadd[d,s] double/single floating-point scalar addition. > +; fccmp[d,s] double/single floating-point conditional compare. Can we follow the convention fcsel uses of calling out "From ARMv8-A:" for this type? Thanks, James
James Greenhalgh <james.greenhalgh@arm.com> wrote: > If we don't have any targets which care about the fccmps/fccmpd split in > the code base, do we really need it? Can we just follow the example of > fcsel? If we do that then we should also change fcmps/d to fcmp to keep the f(c)cmp attributes orthogonal. However it seems better to have all FP operations use {s|d} postfix as the convention (rather than assume that all current and future microarchitectures will treat float and double identically on all operations), so fcsel should ideally be fixed. Wilco
On Thu, Jan 21, 2016 at 11:13:29AM +0000, Wilco Dijkstra wrote: > James Greenhalgh <james.greenhalgh@arm.com> wrote: > > If we don't have any targets which care about the fccmps/fccmpd split in > > the code base, do we really need it? Can we just follow the example of > > fcsel? > > If we do that then we should also change fcmps/d to fcmp to keep the f(c)cmp > attributes orthogonal. However it seems better to have all FP operations use > {s|d} postfix as the convention (rather than assume that all current and future > microarchitectures will treat float and double identically on all operations), > so fcsel should ideally be fixed. Adding values to this type attributes is a pretty lightweight change, and each new type attribute has a small cost in compiler build-time and scheduler performance. Given this, I don't see any need to design for the future, and I don't see why we'd want to add more of them than we need to. The fcmps/fcmpd split is used in cortex-a15-neon.md and cortex-r4f.md so doesn't make a good comparison. If we support a target in future which would benefit from different modeling for fccmps and fccmpd we can split the value then. Thanks, James
Hi, James. On 01/21/16 03:24, James Greenhalgh wrote: > On Wed, Jan 06, 2016 at 02:44:47PM -0600, Evandro Menezes wrote: >> On 01/06/2016 06:04 AM, Wilco Dijkstra wrote: >>>> Here's what I had in mind when I inquired about distinguishing FCMP from >>>> FCCMP. As you can see in the patch, Exynos is the only target that >>>> cares about it, but I wonder if ThunderX or Xgene would too. >>>> >>>> What do you think? >>> The new attributes look fine (I've got a similar outstanding change), however >>> please don't add them to non-AArch64 cores. We only need it for thunderx.md, >>> cortex-a53.md, cortex-a57.md, xgene1.md and exynos-m1.md. >> Add support for the FCCMP insn types >> >> 2016-01-04 Evandro Menezes <e.menezes@samsung.com> >> >> gcc/ >> * config/aarch64/aarch64.md (fccmp): Change insn type. >> (fccmpe): Likewise. >> * config/aarch64/thunderx.md (thunderx_fcmp): Add >> "fccmp{s,d}" types. >> * config/arm/cortex-a53.md (cortex_a53_fpalu): Likewise. >> * config/arm/cortex-a57.md (cortex_a57_fp_cmp): Likewise. >> * config/arm/xgene1.md (xgene1_fcmp): Likewise. >> * config/arm/exynos-m1.md (exynos_m1_fp_ccmp): New insn >> reservation. >> * config/arm/types.md (fccmps): Add new insn type. >> (fccmpd): Likewise. >> >> Got it. Here's an updated patch. Again, assuming that your >> original patch is in place. Perhaps you can build on it. > If we don't have any targets which care about the fccmps/fccmpd split in > the code base, do we really need it? Can we just follow the example of > fcsel? The Exynos M1 does care about the difference between FCMP and FCCMP, as can be seen in the patch. >> diff --git a/gcc/config/arm/types.md b/gcc/config/arm/types.md >> index 321ff89..daf7162 100644 >> --- a/gcc/config/arm/types.md >> +++ b/gcc/config/arm/types.md >> @@ -70,6 +70,7 @@ >> ; f_rint[d,s] double/single floating point rount to integral. >> ; f_store[d,s] double/single store to memory. Used for VFP unit. >> ; fadd[d,s] double/single floating-point scalar addition. >> +; fccmp[d,s] double/single floating-point conditional compare. > Can we follow the convention fcsel uses of calling out "From ARMv8-A:" > for this type? > I'm not sure I follow. Though I didn't refer to the ISA spec, I used the description from it for the *fccmp* type. Please, advise.
On 01/21/16 13:58, Evandro Menezes wrote: > Hi, James. > > On 01/21/16 03:24, James Greenhalgh wrote: >> On Wed, Jan 06, 2016 at 02:44:47PM -0600, Evandro Menezes wrote: >>> On 01/06/2016 06:04 AM, Wilco Dijkstra wrote: >>>>> Here's what I had in mind when I inquired about distinguishing >>>>> FCMP from >>>>> FCCMP. As you can see in the patch, Exynos is the only target that >>>>> cares about it, but I wonder if ThunderX or Xgene would too. >>>>> >>>>> What do you think? >>>> The new attributes look fine (I've got a similar outstanding >>>> change), however >>>> please don't add them to non-AArch64 cores. We only need it for >>>> thunderx.md, >>>> cortex-a53.md, cortex-a57.md, xgene1.md and exynos-m1.md. >>> Add support for the FCCMP insn types >>> >>> 2016-01-04 Evandro Menezes <e.menezes@samsung.com> >>> >>> gcc/ >>> * config/aarch64/aarch64.md (fccmp): Change insn type. >>> (fccmpe): Likewise. >>> * config/aarch64/thunderx.md (thunderx_fcmp): Add >>> "fccmp{s,d}" types. >>> * config/arm/cortex-a53.md (cortex_a53_fpalu): Likewise. >>> * config/arm/cortex-a57.md (cortex_a57_fp_cmp): Likewise. >>> * config/arm/xgene1.md (xgene1_fcmp): Likewise. >>> * config/arm/exynos-m1.md (exynos_m1_fp_ccmp): New insn >>> reservation. >>> * config/arm/types.md (fccmps): Add new insn type. >>> (fccmpd): Likewise. >>> >>> Got it. Here's an updated patch. Again, assuming that your >>> original patch is in place. Perhaps you can build on it. >> If we don't have any targets which care about the fccmps/fccmpd split in >> the code base, do we really need it? Can we just follow the example of >> fcsel? > > The Exynos M1 does care about the difference between FCMP and FCCMP, > as can be seen in the patch. > > More explicitly: (define_insn_reservation "exynos_m1_fp_cmp" 4 (and (eq_attr "tune" "exynosm1") (eq_attr "type" "fcmps, fcmpd")) "em1_nmisc") (define_insn_reservation "exynos_m1_fp_ccmp" 7 (and (eq_attr "tune" "exynosm1") (eq_attr "type" "fccmps, fccmpd")) "em1_st, em1_nmisc") Thank you,
On Thu, Jan 21, 2016 at 01:58:31PM -0600, Evandro Menezes wrote: > Hi, James. > > On 01/21/16 03:24, James Greenhalgh wrote: > >On Wed, Jan 06, 2016 at 02:44:47PM -0600, Evandro Menezes wrote: > >>On 01/06/2016 06:04 AM, Wilco Dijkstra wrote: > >>>>Here's what I had in mind when I inquired about distinguishing FCMP from > >>>>FCCMP. As you can see in the patch, Exynos is the only target that > >>>>cares about it, but I wonder if ThunderX or Xgene would too. > >>>> > >>>>What do you think? > >>>The new attributes look fine (I've got a similar outstanding change), however > >>>please don't add them to non-AArch64 cores. We only need it for thunderx.md, > >>>cortex-a53.md, cortex-a57.md, xgene1.md and exynos-m1.md. > >> Add support for the FCCMP insn types > >> > >> 2016-01-04 Evandro Menezes <e.menezes@samsung.com> > >> > >> gcc/ > >> * config/aarch64/aarch64.md (fccmp): Change insn type. > >> (fccmpe): Likewise. > >> * config/aarch64/thunderx.md (thunderx_fcmp): Add > >> "fccmp{s,d}" types. > >> * config/arm/cortex-a53.md (cortex_a53_fpalu): Likewise. > >> * config/arm/cortex-a57.md (cortex_a57_fp_cmp): Likewise. > >> * config/arm/xgene1.md (xgene1_fcmp): Likewise. > >> * config/arm/exynos-m1.md (exynos_m1_fp_ccmp): New insn > >> reservation. > >> * config/arm/types.md (fccmps): Add new insn type. > >> (fccmpd): Likewise. > >> > >>Got it. Here's an updated patch. Again, assuming that your > >>original patch is in place. Perhaps you can build on it. > >If we don't have any targets which care about the fccmps/fccmpd split in > >the code base, do we really need it? Can we just follow the example of > >fcsel? > > The Exynos M1 does care about the difference between FCMP and FCCMP, > as can be seen in the patch. > More explicitly: > > (define_insn_reservation "exynos_m1_fp_cmp" 4 > (and (eq_attr "tune" "exynosm1") > (eq_attr "type" "fcmps, fcmpd")) > "em1_nmisc") > > (define_insn_reservation "exynos_m1_fp_ccmp" 7 > (and (eq_attr "tune" "exynosm1") > (eq_attr "type" "fccmps, fccmpd")) > "em1_st, em1_nmisc") > I think I was unclear. Your exynos-m1 model cares about splitting fcmp[s/d] and fccmp, but it doesn't care about splitting fccmp in to fccmps/fccmpd. It is the split to fccmps/fccmpd that I think is unneccesary at this time. > >>diff --git a/gcc/config/arm/types.md b/gcc/config/arm/types.md > >>index 321ff89..daf7162 100644 > >>--- a/gcc/config/arm/types.md > >>+++ b/gcc/config/arm/types.md > >>@@ -70,6 +70,7 @@ > >> ; f_rint[d,s] double/single floating point rount to integral. > >> ; f_store[d,s] double/single store to memory. Used for VFP unit. > >> ; fadd[d,s] double/single floating-point scalar addition. > >>+; fccmp[d,s] double/single floating-point conditional compare. > >Can we follow the convention fcsel uses of calling out "From ARMv8-A:" > >for this type? > > > > I'm not sure I follow. Though I didn't refer to the ISA spec, I > used the description from it for the *fccmp* type. > > Please, advise. Something like: ; fccmp From ARMv8-A: floating point conditional compare. Just to capture that this instruction is only available for cores implementing ARMv8-A. Thanks, James
From b5b603880eb265536eb927499c8df3ca6c42e718 Mon Sep 17 00:00:00 2001
From: Evandro Menezes <e.menezes@samsung.com>
Date: Mon, 4 Jan 2016 18:44:30 -0600
Subject: [PATCH] Add support for the FCCMP insn types
2016-01-04 Evandro Menezes <e.menezes@samsung.com>
gcc/
* config/aarch64/aarch64.md (fccmp): Change insn type.
(fccmpe): Likewise.
* config/aarch64/thunderx.md (thunderx_fcmp): Add "fccmp{s,d}" types.
* config/arm/cortex-a53.md (cortex_a53_fpalu): Likewise.
* config/arm/cortex-a57.md (cortex_a57_fp_cmp): Likewise.
* config/arm/xgene1.md (xgene1_fcmp): Likewise.
* config/arm/exynos-m1.md (exynos_m1_fp_ccmp): New insn reservation.
* config/arm/types.md (fccmps): Add new insn type.
(fccmpd): Likewise.
---
gcc/config/aarch64/aarch64.md | 4 ++--
gcc/config/aarch64/thunderx.md | 2 +-
gcc/config/arm/cortex-a53.md | 4 ++--
gcc/config/arm/cortex-a57.md | 2 +-
gcc/config/arm/exynos-m1.md | 5 +++++
gcc/config/arm/types.md | 3 +++
gcc/config/arm/xgene1.md | 2 +-
7 files changed, 15 insertions(+), 7 deletions(-)
diff --git a/gcc/config/aarch64/aarch64.md b/gcc/config/aarch64/aarch64.md
index 8b737bc..9a7ebf4 100644
--- a/gcc/config/aarch64/aarch64.md
+++ b/gcc/config/aarch64/aarch64.md
@@ -335,7 +335,7 @@
(match_operand 5 "immediate_operand")))]
"TARGET_FLOAT"
"fccmp\\t%<s>2, %<s>3, %k5, %m4"
- [(set_attr "type" "fcmp<s>")]
+ [(set_attr "type" "fccmp<s>")]
)
(define_insn "fccmpe<mode>"
@@ -350,7 +350,7 @@
(match_operand 5 "immediate_operand")))]
"TARGET_FLOAT"
"fccmpe\\t%<s>2, %<s>3, %k5, %m4"
- [(set_attr "type" "fcmp<s>")]
+ [(set_attr "type" "fccmp<s>")]
)
;; Expansion of signed mod by a power of 2 using CSNEG.
diff --git a/gcc/config/aarch64/thunderx.md b/gcc/config/aarch64/thunderx.md
index 922df39..058713a 100644
--- a/gcc/config/aarch64/thunderx.md
+++ b/gcc/config/aarch64/thunderx.md
@@ -156,7 +156,7 @@
(define_insn_reservation "thunderx_fcmp" 3
(and (eq_attr "tune" "thunderx")
- (eq_attr "type" "fcmps,fcmpd"))
+ (eq_attr "type" "fcmps,fcmpd,fccmps,fccmpd"))
"thunderx_pipe1")
(define_insn_reservation "thunderx_fmul" 6
diff --git a/gcc/config/arm/cortex-a53.md b/gcc/config/arm/cortex-a53.md
index c1eeedb..fc60bc2 100644
--- a/gcc/config/arm/cortex-a53.md
+++ b/gcc/config/arm/cortex-a53.md
@@ -508,8 +508,8 @@
(define_insn_reservation "cortex_a53_fpalu" 5
(and (eq_attr "tune" "cortexa53")
(eq_attr "type" "ffariths, fadds, ffarithd, faddd, fmov,
- f_cvt, fcmps, fcmpd, fcsel, f_rints, f_rintd,
- f_minmaxs, f_minmaxd"))
+ f_cvt, fcmps, fcmpd, fccmps, fccmpd, fcsel,
+ f_rints, f_rintd, f_minmaxs, f_minmaxd"))
"cortex_a53_slot_any,cortex_a53_fp_alu")
(define_insn_reservation "cortex_a53_fconst" 3
diff --git a/gcc/config/arm/cortex-a57.md b/gcc/config/arm/cortex-a57.md
index 0d28951..f4c112c 100644
--- a/gcc/config/arm/cortex-a57.md
+++ b/gcc/config/arm/cortex-a57.md
@@ -716,7 +716,7 @@
(define_insn_reservation "cortex_a57_fp_cmp" 7
(and (eq_attr "tune" "cortexa57")
- (eq_attr "type" "fcmps,fcmpd"))
+ (eq_attr "type" "fcmps,fcmpd,fccmps,fccmpd"))
"ca57_cx2")
(define_insn_reservation "cortex_a57_fp_arith" 4
diff --git a/gcc/config/arm/exynos-m1.md b/gcc/config/arm/exynos-m1.md
index 0448073..973c8a9 100644
--- a/gcc/config/arm/exynos-m1.md
+++ b/gcc/config/arm/exynos-m1.md
@@ -823,6 +823,11 @@
(eq_attr "type" "fcmps, fcmpd"))
"em1_nmisc")
+(define_insn_reservation "exynos_m1_fp_ccmp" 7
+ (and (eq_attr "tune" "exynosm1")
+ (eq_attr "type" "fccmps, fccmpd"))
+ "em1_st, em1_nmisc")
+
(define_insn_reservation "exynos_m1_fp_sel" 4
(and (eq_attr "tune" "exynosm1")
(eq_attr "type" "fcsel"))
diff --git a/gcc/config/arm/types.md b/gcc/config/arm/types.md
index 321ff89..daf7162 100644
--- a/gcc/config/arm/types.md
+++ b/gcc/config/arm/types.md
@@ -70,6 +70,7 @@
; f_rint[d,s] double/single floating point rount to integral.
; f_store[d,s] double/single store to memory. Used for VFP unit.
; fadd[d,s] double/single floating-point scalar addition.
+; fccmp[d,s] double/single floating-point conditional compare.
; fcmp[d,s] double/single floating-point compare.
; fconst[d,s] double/single load immediate.
; fcsel From ARMv8-A: Floating-point conditional select.
@@ -582,6 +583,8 @@
f_stores,\
faddd,\
fadds,\
+ fccmpd,\
+ fccmps,\
fcmpd,\
fcmps,\
fconstd,\
diff --git a/gcc/config/arm/xgene1.md b/gcc/config/arm/xgene1.md
index 8dfd8a1..b7aeac6 100644
--- a/gcc/config/arm/xgene1.md
+++ b/gcc/config/arm/xgene1.md
@@ -154,7 +154,7 @@
(define_insn_reservation "xgene1_fcmp" 10
(and (eq_attr "tune" "xgene1")
- (eq_attr "type" "fcmpd,fcmps"))
+ (eq_attr "type" "fcmpd,fcmps,fccmpd,fccmps"))
"xgene1_decode1op,xgene1_fsu+xgene1_fcmp*3")
(define_insn_reservation "xgene1_fcsel" 3
--
1.9.1