[committed][PR rtl-optimization/126426] Handle vector mode uses conservatively in ext-dce
Jeffrey Law <[email protected]>
| Newsgroups | gmane.comp.gcc.patches |
|---|---|
| Message-ID | <[email protected]> |
So ext-dce isn't terribly vector aware, it mostly tries to stay out of the way and stay conservatively correct when vector modes are encountered. When that code was added in 2023/2024 it goofed a very important case that has now reared its ugly head. Consider a use of V2HI. What bits are live as a result of such a use? Right now we record 0xffff as the bits potentially live. But that's badly wrong. We have 2 HI objects, so there's actually 32 bits of live data, so 0xffffffff. With the bits live being incorrect we can incorrectly remove an extension because we think the bits are never read. That's precisely what happens in this PR. Not much motivation was given for using GET_MODE_INNER rather than GET_MODE other than it works better for vector/complex. I should have caught this when it was introduced. For a destination, GET_MODE_INNER is safe. The worst case is we'll fail to mark bits as dead which in turn will inhibit optimization. For a source operand it's a completely different story. By failing to mark some bits as live we can (as this pr showed) erroneously remove an extension. While reviewing all the uses of GET_MODE_INNER, I've become convinced the vector handling in carry_backpropagate is broken. If it's going to support vector, it needs to do what it's doing now with the GET_MODE_INNER stuff which gives us liveness of an element, then broadcast the liveness state across all the elements of the vector. It seems like it should be possible to support this case, but having not seen anything even close to it in practice, I'm punting it for now and returning a conservatively correct state when presented with vector modes in carry_backpropagate. This has been bootstrapped and regression tested on riscv64, x86_64, aarch64, alpha, hppa and others. It's also been tested without regressions on the various *-elf targets. Pushing to the trunk. Jeff
0009-pr126426.patch
(text/plain, 4.4 KB)
PR rtl-optimization/126426
gcc/
* ext-dce.cc (carry_backpropagate): Return a conservatively
correct mask when presented with vector modes.
(ext_dce_process_uses): Don't use GET_MODE_INNER, we need to
know the full extent of the bits for vector and complex modes.
gcc/testsuite
* gcc.target/riscv/pr126426.c: New test.
diff --git a/gcc/ext-dce.cc b/gcc/ext-dce.cc
index 9e4b0a429b0..02e7348bf15 100644
--- a/gcc/ext-dce.cc
+++ b/gcc/ext-dce.cc
@@ -920,7 +920,23 @@ carry_backpropagate (unsigned HOST_WIDE_INT mask, enum rtx_code code, rtx x)
if (mask == 0)
return 0;
- enum machine_mode mode = GET_MODE_INNER (GET_MODE (x));
+ /* Consider a vector operation, the bits live are the element bits live
+ broadcasted across the vector. So there can be holes (consider a
+ logical shift).
+
+ Vector modes aren't likely to represent cases we can optimize with
+ any regularity. It seems sensible to just punt that case in a
+ conservatively correct way.
+
+ The conservatively corect choice here would be to return the mode
+ mask for the outer mode. We're already doing that for modes larger
+ than HOST_BITS_PER_WIDE_INT, so it should be safe for larger vectors
+ as well as something like V2HI. */
+ if (VECTOR_MODE_P (GET_MODE (x)) || COMPLEX_MODE_P (GET_MODE (x)))
+ return GET_MODE_MASK (GET_MODE (x));
+
+
+ enum machine_mode mode = GET_MODE (x);
unsigned HOST_WIDE_INT mmask = GET_MODE_MASK (mode);
/* While we don't try to optimize operations on types larger
@@ -994,6 +1010,11 @@ carry_backpropagate (unsigned HOST_WIDE_INT mask, enum rtx_code code, rtx x)
|| !GET_MODE_BITSIZE (GET_MODE (XEXP (x, 0))).is_constant ())
return -1;
+ /* See note about vector modes near the start of this function. */
+ if (VECTOR_MODE_P (GET_MODE (XEXP (x, 0)))
+ || COMPLEX_MODE_P (GET_MODE (XEXP (x, 0))))
+ return GET_MODE_MASK (GET_MODE (XEXP (x, 0)));
+
/* We want the mode of the inner object. We need to ensure its
sign bit is on in MASK. */
mode = GET_MODE_INNER (GET_MODE (XEXP (x, 0)));
@@ -1142,7 +1163,7 @@ ext_dce_process_uses (rtx_insn *insn, rtx obj,
{
rtx inner = XEXP (src, 0);
unsigned HOST_WIDE_INT src_mask
- = GET_MODE_MASK (GET_MODE_INNER (GET_MODE (inner)));
+ = GET_MODE_MASK (GET_MODE (inner));
/* DST_MASK could be zero if we had something in the SET
that we couldn't handle. */
@@ -1335,8 +1356,7 @@ ext_dce_process_uses (rtx_insn *insn, rtx obj,
propagate destination liveness through, then just
set the mask to the mode's mask. */
if (!safe_for_live_propagation (code))
- tmp_mask
- = GET_MODE_MASK (GET_MODE_INNER (GET_MODE (y)));
+ tmp_mask = GET_MODE_MASK (GET_MODE (y));
if (tmp_mask & 0xff)
bitmap_set_bit (livenow, rn);
diff --git a/gcc/testsuite/gcc.target/riscv/pr126426.c b/gcc/testsuite/gcc.target/riscv/pr126426.c
new file mode 100644
index 00000000000..e7819d61d62
--- /dev/null
+++ b/gcc/testsuite/gcc.target/riscv/pr126426.c
@@ -0,0 +1,43 @@
+/* { dg-do run } */
+/* { dg-additional-options "--std=gnu99 -march=rv64gcb" { target rv64 } } */
+/* { dg-additional-options "--std=gnu99 -march=rv32gcb" { target rv32 } } */
+
+typedef union {
+ short mv[2];
+ unsigned mv32;
+} MotionVector;
+enum { kProjectionMvClamp, kMaxFrameDistance };
+short kProjectionMvDivisionLookup[] = {0, 6};
+int SpecGetMvProjectionKernel(int mv, int numerator, int denominator) {
+ int value = mv * numerator * kProjectionMvDivisionLookup[denominator];
+ if (value >= 0)
+ value += 3;
+ value >>= 4;
+ if (value > 4)
+ value = 1;
+ return value;
+}
+void SpecGetMvProjectionNoClamp(MotionVector mv, int numerator, int denominator,
+ MotionVector *projection_mv) {
+ for (int i = 0; i < 2; ++i)
+ projection_mv->mv[i] =
+ SpecGetMvProjectionKernel(mv.mv[i], numerator, denominator);
+}
+int main() {
+ short mvs[5][2] = {{0}};
+ for (int j = 0; j < 5; ++j) {
+ short *mv_value = mvs[j];
+ for (int numerator = -kMaxFrameDistance; numerator; ++numerator)
+ for (int denominator = 0; denominator <= kMaxFrameDistance;
+ ++denominator) {
+ MotionVector mv, spec_projection_mv;
+ mv.mv[0] = mv.mv[1] = mv_value[1];
+ SpecGetMvProjectionNoClamp(mv, numerator, denominator,
+ &spec_projection_mv);
+ if (spec_projection_mv.mv32) {
+ __builtin_abort ();
+ }
+ }
+ }
+ __builtin_exit (0);
+}