diff mbox series

[4/7] RISC-V: Recognize sign-extract + and cases for XVentanaCondOps

Message ID 20221112212943.3068249-5-philipp.tomsich@vrull.eu
State Accepted
Headers
Series RISC-V: Backend support for XVentanaCondOps/ZiCondops |

Checks

Context Check Description
snail/gcc-patch-check success Github commit url

Commit Message

Philipp Tomsich Nov. 12, 2022, 9:29 p.m. UTC 
  Users might use explicit arithmetic operations to create a mask and
then and it, in a sequence like
    cond = (bits >> SHIFT) & 1;
    mask = ~(cond - 1);
    val &= mask;
which will present as a single-bit sign-extract.

Dependening on what combination of XVentanaCondOps and Zbs are
available, this will map to the following sequences:
 - bexti + vt.maskc, if both Zbs and XVentanaCondOps are present
 - andi + vt.maskc, if only XVentanaCondOps is available and the
                    sign-extract is operating on bits 10:0 (bit
		    11 can't be reached, as the immediate is
		    sign-extended)
 - slli + srli + and, otherwise.

gcc/ChangeLog:

	* config/riscv/xventanacondops.md: Recognize SIGN_EXTRACT
	  of a single-bit followed by AND for XVentanaCondOps.

Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
---

 gcc/config/riscv/xventanacondops.md | 46 +++++++++++++++++++++++++++++
 1 file changed, 46 insertions(+)

Comments

Jeff Law Nov. 17, 2022, 11:41 p.m. UTC | #1 
On 11/12/22 14:29, Philipp Tomsich wrote:
> Users might use explicit arithmetic operations to create a mask and
> then and it, in a sequence like
>      cond = (bits >> SHIFT) & 1;
>      mask = ~(cond - 1);
>      val &= mask;
> which will present as a single-bit sign-extract.
>
> Dependening on what combination of XVentanaCondOps and Zbs are
> available, this will map to the following sequences:
>   - bexti + vt.maskc, if both Zbs and XVentanaCondOps are present
>   - andi + vt.maskc, if only XVentanaCondOps is available and the
>                      sign-extract is operating on bits 10:0 (bit
> 		    11 can't be reached, as the immediate is
> 		    sign-extended)
>   - slli + srli + and, otherwise.
>
> gcc/ChangeLog:
>
> 	* config/riscv/xventanacondops.md: Recognize SIGN_EXTRACT
> 	  of a single-bit followed by AND for XVentanaCondOps.
>
> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> ---
>
>   gcc/config/riscv/xventanacondops.md | 46 +++++++++++++++++++++++++++++
>   1 file changed, 46 insertions(+)
>
> diff --git a/gcc/config/riscv/xventanacondops.md b/gcc/config/riscv/xventanacondops.md
> index 7930ef1d837..3e9d5833a4b 100644
> --- a/gcc/config/riscv/xventanacondops.md
> +++ b/gcc/config/riscv/xventanacondops.md
> @@ -73,3 +73,49 @@
>     "TARGET_XVENTANACONDOPS"
>     [(set (match_dup 5) (match_dup 1))
>      (set (match_dup 0) (and:X (neg:X (ne:X (match_dup 5) (const_int 0)))
> +
> +;; Users might use explicit arithmetic operations to create a mask and
> +;; then and it, in a sequence like

Nit.  Seems like a word is missing.  "make and then and it"??


Do we really care about TARGET_XVENTANACONDOPS && ! TARGET_ZBS?


If there's a good reason to care about the !TARGET_ZBS case, then OK 
with the nit fixed.   If we agree that the !TARGET_ZBS case isn't all 
that important, then obviously OK with that pattern removed too.

I'm about out of oomph today.  I may take a look at 7/7 tonight though.  
Given it hits target independent code we probably want to get resolution 
on that patch sooner rather than later.

jeff
Palmer Dabbelt Nov. 17, 2022, 11:56 p.m. UTC | #2 
On Thu, 17 Nov 2022 15:41:26 PST (-0800), gcc-patches@gcc.gnu.org wrote:
>
> On 11/12/22 14:29, Philipp Tomsich wrote:
>> Users might use explicit arithmetic operations to create a mask and
>> then and it, in a sequence like
>>      cond = (bits >> SHIFT) & 1;
>>      mask = ~(cond - 1);
>>      val &= mask;
>> which will present as a single-bit sign-extract.
>>
>> Dependening on what combination of XVentanaCondOps and Zbs are
>> available, this will map to the following sequences:
>>   - bexti + vt.maskc, if both Zbs and XVentanaCondOps are present
>>   - andi + vt.maskc, if only XVentanaCondOps is available and the
>>                      sign-extract is operating on bits 10:0 (bit
>> 		    11 can't be reached, as the immediate is
>> 		    sign-extended)
>>   - slli + srli + and, otherwise.
>>
>> gcc/ChangeLog:
>>
>> 	* config/riscv/xventanacondops.md: Recognize SIGN_EXTRACT
>> 	  of a single-bit followed by AND for XVentanaCondOps.
>>
>> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
>> ---
>>
>>   gcc/config/riscv/xventanacondops.md | 46 +++++++++++++++++++++++++++++
>>   1 file changed, 46 insertions(+)
>>
>> diff --git a/gcc/config/riscv/xventanacondops.md b/gcc/config/riscv/xventanacondops.md
>> index 7930ef1d837..3e9d5833a4b 100644
>> --- a/gcc/config/riscv/xventanacondops.md
>> +++ b/gcc/config/riscv/xventanacondops.md
>> @@ -73,3 +73,49 @@
>>     "TARGET_XVENTANACONDOPS"
>>     [(set (match_dup 5) (match_dup 1))
>>      (set (match_dup 0) (and:X (neg:X (ne:X (match_dup 5) (const_int 0)))
>> +
>> +;; Users might use explicit arithmetic operations to create a mask and
>> +;; then and it, in a sequence like
>
> Nit.  Seems like a word is missing.  "make and then and it"??
>
>
> Do we really care about TARGET_XVENTANACONDOPS && ! TARGET_ZBS?

I guess that's really more of a question for the Ventana folks, but 
assuming all the Ventana widgets have Zbs then it seems reasonable to 
just couple them -- there's already enough options in RISC-V land to 
test everything, might as well make sure what slips through the cracks 
isn't being built.

Probably best to have a comment saying why here, and then something to 
enforce the dependency in -march (either as an implict extension 
dependency, or just a warning/error) so users don't get tripped up on 
configs that aren't expected to work.

> If there's a good reason to care about the !TARGET_ZBS case, then OK
> with the nit fixed.   If we agree that the !TARGET_ZBS case isn't all
> that important, then obviously OK with that pattern removed too.
>
> I'm about out of oomph today.  I may take a look at 7/7 tonight though. 
> Given it hits target independent code we probably want to get resolution
> on that patch sooner rather than later.

Thanks, there's no way we would have gotten this all sorted out so fast 
without the help!
Philipp Tomsich Nov. 18, 2022, 12:08 a.m. UTC | #3 
On Fri, 18 Nov 2022 at 00:41, Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
> On 11/12/22 14:29, Philipp Tomsich wrote:
> > Users might use explicit arithmetic operations to create a mask and
> > then and it, in a sequence like
> >      cond = (bits >> SHIFT) & 1;
> >      mask = ~(cond - 1);
> >      val &= mask;
> > which will present as a single-bit sign-extract.
> >
> > Dependening on what combination of XVentanaCondOps and Zbs are
> > available, this will map to the following sequences:
> >   - bexti + vt.maskc, if both Zbs and XVentanaCondOps are present
> >   - andi + vt.maskc, if only XVentanaCondOps is available and the
> >                      sign-extract is operating on bits 10:0 (bit
> >                   11 can't be reached, as the immediate is
> >                   sign-extended)
> >   - slli + srli + and, otherwise.
> >
> > gcc/ChangeLog:
> >
> >       * config/riscv/xventanacondops.md: Recognize SIGN_EXTRACT
> >         of a single-bit followed by AND for XVentanaCondOps.
> >
> > Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> > ---
> >
> >   gcc/config/riscv/xventanacondops.md | 46 +++++++++++++++++++++++++++++
> >   1 file changed, 46 insertions(+)
> >
> > diff --git a/gcc/config/riscv/xventanacondops.md b/gcc/config/riscv/xventanacondops.md
> > index 7930ef1d837..3e9d5833a4b 100644
> > --- a/gcc/config/riscv/xventanacondops.md
> > +++ b/gcc/config/riscv/xventanacondops.md
> > @@ -73,3 +73,49 @@
> >     "TARGET_XVENTANACONDOPS"
> >     [(set (match_dup 5) (match_dup 1))
> >      (set (match_dup 0) (and:X (neg:X (ne:X (match_dup 5) (const_int 0)))
> > +
> > +;; Users might use explicit arithmetic operations to create a mask and
> > +;; then and it, in a sequence like
>
> Nit.  Seems like a word is missing.  "make and then and it"??
>
>
> Do we really care about TARGET_XVENTANACONDOPS && ! TARGET_ZBS?

While Ventana might not plan to have this combination, nothing
prevents someone to implement only a single one of these — just as
users might choose to override the -march string.  Also note that (the
proposed) ZiCondOps will share most of its infrastructure with
XVentanaCondOps, we will have the same situation there.

> If there's a good reason to care about the !TARGET_ZBS case, then OK
> with the nit fixed.   If we agree that the !TARGET_ZBS case isn't all
> that important, then obviously OK with that pattern removed too.
>
> I'm about out of oomph today.  I may take a look at 7/7 tonight though.
> Given it hits target independent code we probably want to get resolution
> on that patch sooner rather than later.
>
> jeff
>
Philipp Tomsich Nov. 18, 2022, 12:10 a.m. UTC | #4 
On Fri, 18 Nov 2022 at 00:56, Palmer Dabbelt <palmer@rivosinc.com> wrote:
>
> On Thu, 17 Nov 2022 15:41:26 PST (-0800), gcc-patches@gcc.gnu.org wrote:
> >
> > On 11/12/22 14:29, Philipp Tomsich wrote:
> >> Users might use explicit arithmetic operations to create a mask and
> >> then and it, in a sequence like
> >>      cond = (bits >> SHIFT) & 1;
> >>      mask = ~(cond - 1);
> >>      val &= mask;
> >> which will present as a single-bit sign-extract.
> >>
> >> Dependening on what combination of XVentanaCondOps and Zbs are
> >> available, this will map to the following sequences:
> >>   - bexti + vt.maskc, if both Zbs and XVentanaCondOps are present
> >>   - andi + vt.maskc, if only XVentanaCondOps is available and the
> >>                      sign-extract is operating on bits 10:0 (bit
> >>                  11 can't be reached, as the immediate is
> >>                  sign-extended)
> >>   - slli + srli + and, otherwise.
> >>
> >> gcc/ChangeLog:
> >>
> >>      * config/riscv/xventanacondops.md: Recognize SIGN_EXTRACT
> >>        of a single-bit followed by AND for XVentanaCondOps.
> >>
> >> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> >> ---
> >>
> >>   gcc/config/riscv/xventanacondops.md | 46 +++++++++++++++++++++++++++++
> >>   1 file changed, 46 insertions(+)
> >>
> >> diff --git a/gcc/config/riscv/xventanacondops.md b/gcc/config/riscv/xventanacondops.md
> >> index 7930ef1d837..3e9d5833a4b 100644
> >> --- a/gcc/config/riscv/xventanacondops.md
> >> +++ b/gcc/config/riscv/xventanacondops.md
> >> @@ -73,3 +73,49 @@
> >>     "TARGET_XVENTANACONDOPS"
> >>     [(set (match_dup 5) (match_dup 1))
> >>      (set (match_dup 0) (and:X (neg:X (ne:X (match_dup 5) (const_int 0)))
> >> +
> >> +;; Users might use explicit arithmetic operations to create a mask and
> >> +;; then and it, in a sequence like
> >
> > Nit.  Seems like a word is missing.  "make and then and it"??
> >
> >
> > Do we really care about TARGET_XVENTANACONDOPS && ! TARGET_ZBS?
>
> I guess that's really more of a question for the Ventana folks, but
> assuming all the Ventana widgets have Zbs then it seems reasonable to
> just couple them -- there's already enough options in RISC-V land to
> test everything, might as well make sure what slips through the cracks
> isn't being built.
>
> Probably best to have a comment saying why here, and then something to
> enforce the dependency in -march (either as an implict extension
> dependency, or just a warning/error) so users don't get tripped up on
> configs that aren't expected to work.

With an eye to (the proposed) ZiCondOps, I'd rather pull this in once
XVentanaCondOps is applied.
That said, we'll need to add a test-case for these.

> > If there's a good reason to care about the !TARGET_ZBS case, then OK
> > with the nit fixed.   If we agree that the !TARGET_ZBS case isn't all
> > that important, then obviously OK with that pattern removed too.
> >
> > I'm about out of oomph today.  I may take a look at 7/7 tonight though.
> > Given it hits target independent code we probably want to get resolution
> > on that patch sooner rather than later.
>
> Thanks, there's no way we would have gotten this all sorted out so fast
> without the help!
Jeff Law Nov. 18, 2022, 2:34 p.m. UTC | #5 
On 11/17/22 16:56, Palmer Dabbelt wrote:
> On Thu, 17 Nov 2022 15:41:26 PST (-0800), gcc-patches@gcc.gnu.org wrote:
>>
>> On 11/12/22 14:29, Philipp Tomsich wrote:
>>> Users might use explicit arithmetic operations to create a mask and
>>> then and it, in a sequence like
>>>      cond = (bits >> SHIFT) & 1;
>>>      mask = ~(cond - 1);
>>>      val &= mask;
>>> which will present as a single-bit sign-extract.
>>>
>>> Dependening on what combination of XVentanaCondOps and Zbs are
>>> available, this will map to the following sequences:
>>>   - bexti + vt.maskc, if both Zbs and XVentanaCondOps are present
>>>   - andi + vt.maskc, if only XVentanaCondOps is available and the
>>>                      sign-extract is operating on bits 10:0 (bit
>>>             11 can't be reached, as the immediate is
>>>             sign-extended)
>>>   - slli + srli + and, otherwise.
>>>
>>> gcc/ChangeLog:
>>>
>>>     * config/riscv/xventanacondops.md: Recognize SIGN_EXTRACT
>>>       of a single-bit followed by AND for XVentanaCondOps.
>>>
>>> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
>>> ---
>>>
>>>   gcc/config/riscv/xventanacondops.md | 46 
>>> +++++++++++++++++++++++++++++
>>>   1 file changed, 46 insertions(+)
>>>
>>> diff --git a/gcc/config/riscv/xventanacondops.md 
>>> b/gcc/config/riscv/xventanacondops.md
>>> index 7930ef1d837..3e9d5833a4b 100644
>>> --- a/gcc/config/riscv/xventanacondops.md
>>> +++ b/gcc/config/riscv/xventanacondops.md
>>> @@ -73,3 +73,49 @@
>>>     "TARGET_XVENTANACONDOPS"
>>>     [(set (match_dup 5) (match_dup 1))
>>>      (set (match_dup 0) (and:X (neg:X (ne:X (match_dup 5) (const_int 
>>> 0)))
>>> +
>>> +;; Users might use explicit arithmetic operations to create a mask and
>>> +;; then and it, in a sequence like
>>
>> Nit.  Seems like a word is missing.  "make and then and it"??
>>
>>
>> Do we really care about TARGET_XVENTANACONDOPS && ! TARGET_ZBS?
>
> I guess that's really more of a question for the Ventana folks, but 
> assuming all the Ventana widgets have Zbs then it seems reasonable to 
> just couple them -- there's already enough options in RISC-V land to 
> test everything, might as well make sure what slips through the cracks 
> isn't being built.

I'm pretty confident Ventana won't be making a part without Zbs which is 
why I raised the issue


I also understand Philipp's position that one could explicitly turn on 
ventanacondops and zbs off and that there's a notable possibility that 
this ultimately turns into ZICondOps independent of Ventana.


So I guess we keep it...  But it also feels like a ticking time bomb WRT 
the ability to mix and match things the way we currently allow.  I 
suspect if we were to look at the full test matrix and deeply test that 
full matrix that we'd find a number of problems where two options 
interact badly.

Jeff
Philipp Tomsich Nov. 18, 2022, 2:41 p.m. UTC | #6 
On Fri, 18 Nov 2022 at 15:34, Jeff Law <jeffreyalaw@gmail.com> wrote:
>
>
> On 11/17/22 16:56, Palmer Dabbelt wrote:
> > On Thu, 17 Nov 2022 15:41:26 PST (-0800), gcc-patches@gcc.gnu.org wrote:
> >>
> >> On 11/12/22 14:29, Philipp Tomsich wrote:
> >>> Users might use explicit arithmetic operations to create a mask and
> >>> then and it, in a sequence like
> >>>      cond = (bits >> SHIFT) & 1;
> >>>      mask = ~(cond - 1);
> >>>      val &= mask;
> >>> which will present as a single-bit sign-extract.
> >>>
> >>> Dependening on what combination of XVentanaCondOps and Zbs are
> >>> available, this will map to the following sequences:
> >>>   - bexti + vt.maskc, if both Zbs and XVentanaCondOps are present
> >>>   - andi + vt.maskc, if only XVentanaCondOps is available and the
> >>>                      sign-extract is operating on bits 10:0 (bit
> >>>             11 can't be reached, as the immediate is
> >>>             sign-extended)
> >>>   - slli + srli + and, otherwise.
> >>>
> >>> gcc/ChangeLog:
> >>>
> >>>     * config/riscv/xventanacondops.md: Recognize SIGN_EXTRACT
> >>>       of a single-bit followed by AND for XVentanaCondOps.
> >>>
> >>> Signed-off-by: Philipp Tomsich <philipp.tomsich@vrull.eu>
> >>> ---
> >>>
> >>>   gcc/config/riscv/xventanacondops.md | 46
> >>> +++++++++++++++++++++++++++++
> >>>   1 file changed, 46 insertions(+)
> >>>
> >>> diff --git a/gcc/config/riscv/xventanacondops.md
> >>> b/gcc/config/riscv/xventanacondops.md
> >>> index 7930ef1d837..3e9d5833a4b 100644
> >>> --- a/gcc/config/riscv/xventanacondops.md
> >>> +++ b/gcc/config/riscv/xventanacondops.md
> >>> @@ -73,3 +73,49 @@
> >>>     "TARGET_XVENTANACONDOPS"
> >>>     [(set (match_dup 5) (match_dup 1))
> >>>      (set (match_dup 0) (and:X (neg:X (ne:X (match_dup 5) (const_int
> >>> 0)))
> >>> +
> >>> +;; Users might use explicit arithmetic operations to create a mask and
> >>> +;; then and it, in a sequence like
> >>
> >> Nit.  Seems like a word is missing.  "make and then and it"??
> >>
> >>
> >> Do we really care about TARGET_XVENTANACONDOPS && ! TARGET_ZBS?
> >
> > I guess that's really more of a question for the Ventana folks, but
> > assuming all the Ventana widgets have Zbs then it seems reasonable to
> > just couple them -- there's already enough options in RISC-V land to
> > test everything, might as well make sure what slips through the cracks
> > isn't being built.
>
> I'm pretty confident Ventana won't be making a part without Zbs which is
> why I raised the issue
>
>
> I also understand Philipp's position that one could explicitly turn on
> ventanacondops and zbs off and that there's a notable possibility that
> this ultimately turns into ZICondOps independent of Ventana.
>
>
> So I guess we keep it...  But it also feels like a ticking time bomb WRT
> the ability to mix and match things the way we currently allow.  I
> suspect if we were to look at the full test matrix and deeply test that
> full matrix that we'd find a number of problems where two options
> interact badly.

I have been worrying about the exponential growth of the test matrix
for 2 years now and still haven't come up with a good solution. It is
clear that this is a challenge for the entire RISC-V ecosystem and
that it needs to be addressed across vendors and across the entire
membership: unfortunately, that doesn't make for an easier path to a
solution.

And just as an aside: pure extensions are still less worrisome than
subtractive changes (think Zfinx and Zdinx), or the fact that we have
different options for the memory model (RVWMO vs. Ztso), or variations
in regard to what facilities are available for atomics...


Philipp.
diff mbox series

Patch

diff --git a/gcc/config/riscv/xventanacondops.md b/gcc/config/riscv/xventanacondops.md
index 7930ef1d837..3e9d5833a4b 100644
--- a/gcc/config/riscv/xventanacondops.md
+++ b/gcc/config/riscv/xventanacondops.md
@@ -73,3 +73,49 @@ 
   "TARGET_XVENTANACONDOPS"
   [(set (match_dup 5) (match_dup 1))
    (set (match_dup 0) (and:X (neg:X (ne:X (match_dup 5) (const_int 0)))
+
+;; Users might use explicit arithmetic operations to create a mask and
+;; then and it, in a sequence like
+;;    cond = (bits >> SHIFT) & 1;
+;;    mask = ~(cond - 1);
+;;    val &= mask;
+;; which will present as a single-bit sign-extract in the combiner.
+;;
+;; This will give rise to any of the following cases:
+;; - with Zbs and XVentanaCondOps: bexti + vt.maskc
+;; - with XVentanaCondOps (but w/o Zbs):
+;;   - andi + vt.maskc, if the mask is representable in the immediate
+;;                      (which requires extra care due to the immediate
+;;                       being sign-extended)
+;;   - slli + srli + and
+;; - otherwise: slli + srli + and
+
+;; With Zbb, we have bexti for all possible bits...
+(define_split
+  [(set (match_operand:X 0 "register_operand")
+	(and:X (sign_extract:X (match_operand:X 1 "register_operand")
+			       (const_int 1)
+			       (match_operand 2 "immediate_operand"))
+	       (match_operand:X 3 "register_operand")))
+   (clobber (match_operand:X 4 "register_operand"))]
+  "TARGET_XVENTANACONDOPS && TARGET_ZBS"
+  [(set (match_dup 4) (zero_extract:X (match_dup 1) (const_int 1) (match_dup 2)))
+   (set (match_dup 0) (and:X (neg:X (ne:X (match_dup 4) (const_int 0)))
+			     (match_dup 3)))])
+
+;; ...whereas RV64I only allows us access to bits 0..10 in a single andi.
+(define_split
+  [(set (match_operand:X 0 "register_operand")
+	(and:X (sign_extract:X (match_operand:X 1 "register_operand")
+			       (const_int 1)
+			       (match_operand 2 "immediate_operand"))
+	       (match_operand:X 3 "register_operand")))
+   (clobber (match_operand:X 4 "register_operand"))]
+  "TARGET_XVENTANACONDOPS && !TARGET_ZBS && (UINTVAL (operands[2]) < 11)"
+  [(set (match_dup 4) (and:X (match_dup 1) (match_dup 2)))
+   (set (match_dup 0) (and:X (neg:X (ne:X (match_dup 4) (const_int 0)))
+			     (match_dup 3)))]
+{
+  operands[2] = GEN_INT(1 << UINTVAL(operands[2]));
+})
+