This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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
--


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]