On 3/27/23 00:59, [email protected] wrote:
From: Juzhe-Zhong <[email protected]>
PR 108270
Fix bug: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108270.
Consider the following testcase:
void f (void * restrict in, void * restrict out, int l, int n, int m)
{
for (int i = 0; i < l; i++){
for (int j = 0; j < m; j++){
for (int k = 0; k < n; k++)
{
vint8mf8_t v = __riscv_vle8_v_i8mf8 (in + i + j, 17);
__riscv_vse8_v_i8mf8 (out + i + j, v, 17);
}
}
}
}
Compile option: -O3
Before this patch:
mv a7,a2
mv a6,a0
mv t1,a1
mv a2,a3
vsetivli zero,17,e8,mf8,ta,ma
...
After this patch:
mv a7,a2
mv a6,a0
mv t1,a1
mv a2,a3
ble a7,zero,.L1
ble a4,zero,.L1
ble a3,zero,.L1
add a1,a0,a4
li a0,0
vsetivli zero,17,e8,mf8,ta,ma
...
It will produce potential bug when:
int main ()
{
vsetivli zero, 100,.....
f (in, out, 0,0,0)
asm volatile ("csrr a0,vl":::"memory");
// Before this patch the a0 is 17. (Wrong).
// After this patch the a0 is 100. (Correct).
...
}
gcc/ChangeLog:
* config/riscv/riscv-vsetvl.cc
(vector_infos_manager::all_empty_predecessor_p): New function.
(pass_vsetvl::backward_demand_fusion): Fix bug.
* config/riscv/riscv-vsetvl.h: New function declare.
gcc/testsuite/ChangeLog:
* gcc.target/riscv/rvv/vsetvl/imm_bb_prop-1.c: Adapt test.
* gcc.target/riscv/rvv/vsetvl/imm_conflict-3.c: Adapt test.
* gcc.target/riscv/rvv/vsetvl/pr108270.c: New test.
---
gcc/config/riscv/riscv-vsetvl.cc | 24 +++++++++++++++++++
gcc/config/riscv/riscv-vsetvl.h | 2 ++
.../riscv/rvv/vsetvl/imm_bb_prop-1.c | 2 +-
.../riscv/rvv/vsetvl/imm_conflict-3.c | 4 ++--
.../gcc.target/riscv/rvv/vsetvl/pr108270.c | 19 +++++++++++++++
5 files changed, 48 insertions(+), 3 deletions(-)
create mode 100644 gcc/testsuite/gcc.target/riscv/rvv/vsetvl/pr108270.c
diff --git a/gcc/config/riscv/riscv-vsetvl.cc b/gcc/config/riscv/riscv-vsetvl.cc
index b5f5301ea43..4948e5d4c5e 100644
--- a/gcc/config/riscv/riscv-vsetvl.cc
+++ b/gcc/config/riscv/riscv-vsetvl.cc
@@ -2361,6 +2361,21 @@ vector_infos_manager::all_same_ratio_p (sbitmap bitdata)
const
return true;
}
+bool
+vector_infos_manager::all_empty_predecessor_p (const basic_block cfg_bb) const
Needs a function comment. Perhaps something like:
/* Return TRUE if CFG_BB's predecessors have no vector configuration
state. FALSE otherwise. */
Which I think argues that the name isn't good. Perhaps
"no_vector_state_in_preds" would be a better name?
+ /* Fix PR108270:
+
+ bb 0 -> bb 1
+ We don't need to backward fuse VL/VTYPE info from bb 1 to bb 0
+ if bb 1 is not inside a loop and all predecessors of bb 0 are empty. */
+ if (m_vector_manager->all_empty_predecessor_p (cfg_bb))
+ continue;
Rather than "empty" I would say something about vector configuration
state. "empty" is much more likely to be interpreted as having no
instructions or something similar, which isn't the property you're checking.
So I think making the minor comment/name changes and this will be fine.
Please repost it though for a final ACK.
jeff
