Re: Re: [PATCH] RISC-V: Disable user vsetvl fusion into EMPTY block

2023-08-28 Thread juzhe.zh...@rivai.ai 
Address comments:
https://gcc.gnu.org/pipermail/gcc-patches/2023-August/628568.html 




juzhe.zh...@rivai.ai
 
From: Kito Cheng
Date: 2023-08-28 16:58
To: Robin Dapp
CC: Juzhe-Zhong; gcc-patches; kito.cheng
Subject: Re: [PATCH] RISC-V: Disable user vsetvl fusion into EMPTY block
Is it possible to skip that at the topper level like that?
 
diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 682f795c8e1..654d25de593 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -3269,7 +3269,7 @@ pass_vsetvl::earliest_fusion (void)
  for (size_t i = 0; i < m_vector_manager->vector_exprs.length (); i++)
   {
 auto &expr = *m_vector_manager->vector_exprs[i];
- if (expr.empty_p ())
+ if (expr.empty_p () || vsetvl_insn_p (expr.get_insn ()->rtl ()))
   continue;
 edge eg = INDEX_EDGE (m_vector_manager->vector_edge_list, ed);
 if (eg->src == ENTRY_BLOCK_PTR_FOR_FN (cfun)

Re: Re: [PATCH] RISC-V: Disable user vsetvl fusion into EMPTY block

2023-08-28 Thread Kito Cheng via Gcc-patches 
What about that? I guess I don't really know how to determine if a
block is EMPTY?

diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 682f795c8e1..784ab184c72 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -3282,6 +3282,10 @@ pass_vsetvl::earliest_fusion (void)
 if (src_block_info.reaching_out.unknown_p ())
   break;

+ if (vsetvl_insn_p (expr.get_insn ()->rtl ())
+ && src_block_info.reaching_out.empty_p ())
+   continue;
+
 gcc_assert (!(eg->flags & EDGE_ABNORMAL));
 vector_insn_info new_info = vector_insn_info ();
 profile_probability prob = src_block_info.probability;

On Mon, Aug 28, 2023 at 5:09 PM juzhe.zh...@rivai.ai
 wrote:
>
> No. we can't.
>
> My goal is to forbid this following situation:
>
> bb 2:
> EMPTY (no any vsetvl and rvv insn)
> bb 3:
> user vsetvl.
>
> We forbid user vsetvl (bb 3) be move to bb 2 in earliest fusion since user 
> vsetvl has no side effects
> and we believe user vsetvl in bb 3 here is early enough.
>
> However, if we skip that as your patch, we will end up with missing this 
> optimization:
>
> bb 2:
> user vsetvl a5, a4, e8, mf4...
> vle8
> vse8
>
> bb 3:
> user vsetvl a5, a4, e16, mf2... -> In phase 1 && phase 2, the local 
> demand fusion will update it into (e32, m1)
> vle32
> vadd
> vse32
>
> If we skip at the top, we will be missing fuse user vsetvl (in bb 3 e32 m1) 
> into user vsetvl (in bb 2 e8 mf4).
>
> Thanks.
>
>
> juzhe.zh...@rivai.ai
>
> From: Kito Cheng
> Date: 2023-08-28 16:58
> To: Robin Dapp
> CC: Juzhe-Zhong; gcc-patches; kito.cheng
> Subject: Re: [PATCH] RISC-V: Disable user vsetvl fusion into EMPTY block
> Is it possible to skip that at the topper level like that?
>
> diff --git a/gcc/config/riscv/riscv-vsetvl.cc 
> b/gcc/config/riscv/riscv-vsetvl.cc
> index 682f795c8e1..654d25de593 100644
> --- a/gcc/config/riscv/riscv-vsetvl.cc
> +++ b/gcc/config/riscv/riscv-vsetvl.cc
> @@ -3269,7 +3269,7 @@ pass_vsetvl::earliest_fusion (void)
>   for (size_t i = 0; i < m_vector_manager->vector_exprs.length (); i++)
>{
>  auto &expr = *m_vector_manager->vector_exprs[i];
> - if (expr.empty_p ())
> + if (expr.empty_p () || vsetvl_insn_p (expr.get_insn ()->rtl ()))
>continue;
>  edge eg = INDEX_EDGE (m_vector_manager->vector_edge_list, ed);
>  if (eg->src == ENTRY_BLOCK_PTR_FOR_FN (cfun)
>

Re: Re: [PATCH] RISC-V: Disable user vsetvl fusion into EMPTY block

2023-08-28 Thread juzhe.zh...@rivai.ai 
No. we can't.

My goal is to forbid this following situation:

bb 2:
EMPTY (no any vsetvl and rvv insn)
bb 3:
user vsetvl.

We forbid user vsetvl (bb 3) be move to bb 2 in earliest fusion since user 
vsetvl has no side effects
and we believe user vsetvl in bb 3 here is early enough.

However, if we skip that as your patch, we will end up with missing this 
optimization:

bb 2:
user vsetvl a5, a4, e8, mf4...
vle8
vse8

bb 3:
user vsetvl a5, a4, e16, mf2... -> In phase 1 && phase 2, the local demand 
fusion will update it into (e32, m1)
vle32
vadd
vse32

If we skip at the top, we will be missing fuse user vsetvl (in bb 3 e32 m1) 
into user vsetvl (in bb 2 e8 mf4).

Thanks. 


juzhe.zh...@rivai.ai
 
From: Kito Cheng
Date: 2023-08-28 16:58
To: Robin Dapp
CC: Juzhe-Zhong; gcc-patches; kito.cheng
Subject: Re: [PATCH] RISC-V: Disable user vsetvl fusion into EMPTY block
Is it possible to skip that at the topper level like that?
 
diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 682f795c8e1..654d25de593 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -3269,7 +3269,7 @@ pass_vsetvl::earliest_fusion (void)
  for (size_t i = 0; i < m_vector_manager->vector_exprs.length (); i++)
   {
 auto &expr = *m_vector_manager->vector_exprs[i];
- if (expr.empty_p ())
+ if (expr.empty_p () || vsetvl_insn_p (expr.get_insn ()->rtl ()))
   continue;
 edge eg = INDEX_EDGE (m_vector_manager->vector_edge_list, ed);
 if (eg->src == ENTRY_BLOCK_PTR_FOR_FN (cfun)

Re: [PATCH] RISC-V: Disable user vsetvl fusion into EMPTY block

2023-08-28 Thread Kito Cheng via Gcc-patches 
Is it possible to skip that at the topper level like that?

diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index 682f795c8e1..654d25de593 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -3269,7 +3269,7 @@ pass_vsetvl::earliest_fusion (void)
  for (size_t i = 0; i < m_vector_manager->vector_exprs.length (); i++)
   {
 auto &expr = *m_vector_manager->vector_exprs[i];
- if (expr.empty_p ())
+ if (expr.empty_p () || vsetvl_insn_p (expr.get_insn ()->rtl ()))
   continue;
 edge eg = INDEX_EDGE (m_vector_manager->vector_edge_list, ed);
 if (eg->src == ENTRY_BLOCK_PTR_FOR_FN (cfun)

Re: [PATCH] RISC-V: Disable user vsetvl fusion into EMPTY block

2023-08-28 Thread Robin Dapp via Gcc-patches 
> || vsetvl_insn_p (expr.get_insn ()->rtl ()))
>   continue;
> new_info = expr.global_merge (expr, eg->src->index);
> @@ -3317,6 +3335,25 @@ pass_vsetvl::earliest_fusion (void)
> prob = profile_probability::uninitialized ();
>   }
> else if (!src_block_info.reaching_out.compatible_p (expr)
> +/* We don't do fusion across BBs for user explicit
> +   vsetvl instruction for these following reasons:
> +
> +- The user vsetvl instruction is configured as
> +  no side effects that the previous passes
> +  (GSCE, Loop-invariant, ..., etc)
> +  should be able to do a good job on optimization
> +  of user explicit vsetvls so we don't need to
> +  PRE optimization (The user vsetvls should be
> +  on the optimal local already before this pass)
> +  again for user vsetvls in VSETVL PASS here
> +  (Phase 3 && Phase 4).
> +
> +- Allowing user vsetvls be optimized in PRE
> +  optimization here (Phase 3 && Phase 4) will
> +  complicate the codes so much so we prefer user
> +  vsetvls be optimized in post-optimization
> +  (Phase 5 && Phase 6).  */
> +&& !vsetvl_insn_p (expr.get_insn ()->rtl ())
>  && dest_block_info.probability
>   > src_block_info.probability)

This is OK but do we need the same comment twice?  The first one doesn't
seem to refer to a change but also to vsetvl_insn_p (expr.get_insn()...).

And where is the !empty block property enforced?  I would prefer to have
the longer comment at a top level and shortly describe here why
!vsetvl_insn_p ensures we don't have an EMPTY block.  Don't we usually
have a state for this (i.e. empty_p)?

No need for a V2 though, it can also stay as is and be cleaned up later.

Regards
 Robin