On 10/8/24 2:11 PM, Jeff Law wrote:
On 10/2/24 6:27 AM, Dusan Stojkovic wrote:
This patch is a new version of:
https://gcc.gnu.org/pipermail/gcc-patches/2024-September/662745.html
<https://gcc.gnu.org/pipermail/gcc-patches/2024-September/662745.html>
> Can you elaborate a bit on that? Rearranging the CFG shouldn't matter
> in general and relying on the specific TARGET_SFB_ALU feels overly
> specific.
> Why does the same register in the if_then_else and interfere with
vsetvl?
When ce1 pass transforms CFG in the case of the conditional move, it
deletes
then and else basic blocks and in their place adds the conditional
move which
uses the same pseudo-register as the original vsetvl.
This interferes with vsetvl pass precisely because of the merge
policy. Use by
non rvv flag limits the cases where merging might still be possible.
This patch
tries to addresses one such issue.
Agreed. I have removed TARGET_SFB_ALU flag from the condition.
> BTW Bohan Lei has since fixed a bug regarding non-RVV uses. Does the
> situation change with that applied?
Repeated the testing for sifive-7-series as well as rocket. The same
tests are
still effected positively: vsetvlmax-9, vsetvlmax-10, vsetvlmax-11,
vsetvlmax-15
on sifive-7-series.
2024-10-2 Dusan Stojkovic <dusan.stojko...@rt-rk.com>
PR target/113035
gcc/ChangeLog:
* config/riscv/riscv-vsetvl.cc
(pre_vsetvl::earliest_fuse_vsetvl_info):
New fuse condition.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/vsetvl/vsetvlmax-15.c: Updated
scan-assembler-times num parameter.
So the meat of this patch is the introduction of this hunk of code
(formatting fixed):
if (prev_info.valid_p () &&
curr_info.valid_p ()
&& prev_info.vl_used_by_non_rvv_insn_p ()
&& !curr_info.vl_used_by_non_rvv_insn_p ())
{
// Try to merge each demand individually
if (m_dem.sew_lmul_compatible_p (prev_info, curr_info))
m_dem.merge_sew_lmul (prev_info, curr_info);
if (m_dem.policy_compatible_p (prev_info, curr_info))
m_dem.merge_policy (prev_info, curr_info);
if (dump_file && (dump_flags & TDF_DETAILS))
{
fprintf (dump_file, " After fusing curr info
and "
"prev info demands
individually:\n");
fprintf (dump_file, " prev_info: ");
prev_info.dump (dump_file, " ");
fprintf (dump_file, " curr_info: ");
curr_info.dump (dump_file, " ");
}
}
So why test used_by_non_rvv_insn_p here? Isn't it generally profitable
to go ahead and merge the sew/lmul and policy demands? Or does doing so
interfere with later code in this function?
Should we be setting "changed = true" when we're able to merge sew_lmul
or the merge policy?
So I was working through this patch again. I'm pretty confident this
should be setting changed to true once we do a merge, even in this more
limited case. Essentially that will trigger another iteration allowing
the merged data to potentially bubble up further.
If I understand everything correctly, I don't think we need to be
checking the state of vl_used_by_non_rvv_insn_p here. If that info
differs, we'll reject full merging in the avl check.
Robin, it sounded like you had correctness concerns on the call today.
Can you expand on those?
So this is really the biggest question in my mind. When we kicked this
around in the patchwork meeting several weeks ago I got the impression
Robin had a correctness concern with this code. Robin, do you remember
what had you worried?
Jeff
