On 10/02/13 02:21, Bill Schmidt wrote:
On Tue, 2013-10-01 at 23:57 +0100, Yufeng Zhang wrote:
On 10/01/13 20:55, Bill Schmidt wrote:
On Tue, 2013-10-01 at 11:56 -0500, Bill Schmidt wrote:
OK, thanks. The problem that you've encountered is that you are
attempting to do something illegal. ;) (Bin's original patch is
actually to blame for that, as well as me for not catching it then.)
As your new test shows, it is unsafe to do the transformation in
backtrace_base_for_ref when widening from an unsigned type, because the
unsigned type has wrap semantics by default. (The actual test must be
done on TYPE_OVERFLOW_WRAPS since this wrap semantics can be added or
removed by compile option -- see the comments with legal_cast_p and
legal_cast_p_1 later in the module.)
You cannot in general prove that the transformation is allowable for a
specific constant, because you don't know that what you're adding it to
won't cause an overflow that's handled incorrectly.
I believe the correct fix for the unsigned-overflow case is to fail
backtrace_base_for_ref if legal_cast_p (in_type, out_type) returns
false, where in_type is the type of the new *PBASE, and out_type is the
widening type that you're looking through. So you can't just
STRIP_NOPS, you have to check the cast for legitimacy for this
transformation.
This does not explain why backtrace_base_for_ref does not find all the
opportunities on slsr-39.c. I don't immediately see what's preventing
that. Note that the transformation is legal in that case because you
are widening from a signed int to an unsigned int, which won't cause
problems. You guys need to dig deeper into why those opportunities are
missed when sizetype is larger than int. Let me know if you need help
figuring it out.
Sorry, I had to leave before and wanted to get this response back to you
in case I didn't get back soon. I've looked at this some more, and your
general approach should work ok once you get the legal_cast_p check in
place where you do the get_unwidened call now. Once you know you have a
legal widening, you don't have to worry about the safe_to_multiply_p
stuff. I.e., you don't need the last two chunks in the patch to
backtrace_base_for_ref, and you don't need the unwidened_p variable. It
should all fall out properly by just restricting your unwidening to
legal casts.
Many thanks for looking into the issue so promptly. I've updated the
patch; I have to use legal_cast_p_1 instead as the gimple node is no
longer available by then.
Does the new patch look sane?
Yes, much better. I'm happy with this approach.
Great! The regtest and bootstrap all passed so I've committed the patch.
However, please
restore the correct whitespace before the { at -786,7 +795,7.
This is actually a correction to the whitespace. I've split the patch
and committed it separately.
Thanks again for helping out!
Regards,
Yufeng
