This is the mail archive of the
gcc-patches@gcc.gnu.org
mailing list for the GCC project.
Re: PR rtl-optimization/26244 (incorrect shift optimization)
- From: Roger Sayle <roger at eyesopen dot com>
- To: John David Anglin <dave at hiauly1 dot hia dot nrc dot ca>
- Cc: gcc-patches at gcc dot gnu dot org
- Date: Wed, 2 Aug 2006 14:22:12 -0600 (MDT)
- Subject: Re: PR rtl-optimization/26244 (incorrect shift optimization)
Hi Dave,
On Wed, 2 Aug 2006, John David Anglin wrote:
> The enclosed patch fixes PR rtl-optimization/26244. This bug was
> exposed by changes to the loop optimizer. The problem is that on
> targets where SHIFT_COUNT_TRUNCATED is true the sum of the associated
> shift counts can wrap. In the case of the bug, we had 1 + -1. The
> new shift was computed to be 0. But actually because shift counts
> are truncated, the new shift count should have been 32 and cse
> shouldn't have associated the shifts.
Hmm. The first worrying thing for me is the semantics of a shift by
a negative bit count. There's also the cryptic comment in GCC's RTL
documentation that TARGET_SHIFT_TRUNCATION_MASK doesn't apply to all
rtxes!
I'm also a little confused why a shift of 32 should behave differently
to a shift by zero on a SHIFT_COUNT_TRUNCATED platform. I'd have thought
that 1 + -1 = 0, should when truncated be equivalent to (1 + 31) % 32 = 0.
Perhaps some hardware treats "x << 32" differently from "x << 0", relying
on the compiler to optimize away all "x << 0"?
I do completely agree that much of your patch is correct; that we
do need some kind of test here for SHIFT_COUNT_TRUNCATED, and the
returning of CONST0_RTX is also good (perhaps with an "if (!side_effects
(XEXP (y, 0))" test to make sure we're not throwing a way a volatile
memory reference).
But there are a just one or two missing details, that make me curious
about whether your fix is correct.
As a first step, presumably on PA which defines SHIFT_COUNT_TRUNCATED,
could you confirm that const_arg0 being -1 really means that we should
be shifting by 31 bits? It looks like the PA backend doesn't define
TARGET_SHIFT_TRUNCATION_MASK, so "(x << 1) << -1" is the same as
"(x << 1) << 31" which should be the same as "x << 0".
For example, I'd be a little happier if instead of the modulo tests
you've added, if instead we checked that
&& INTVAL (const_arg1) >= 0
&& INTVAL (inner_cost) >= 0
Dave, unfortunately I'm off on vacation tomorrow for two and a half
weeks to the South of England without internet access, but I'll try
to respond as quickly as possible to your replies before I go.
Roger
--