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. However, please restore the correct whitespace before the { at -786,7 +795,7. Thanks for fixing this up! Bill > > The regtest on aarch64 and bootstrapping on x86-64 are still running. > > Thanks, > Yufeng > > > gcc/ > > * gimple-ssa-strength-reduction.c (legal_cast_p_1): Forward > declaration. > (backtrace_base_for_ref): Call get_unwidened with 'base_in' if > 'base_in' represent a conversion and legal_cast_p_1 holds; set > 'base_in' with the returned value from get_unwidened. > > gcc/testsuite/ > > * gcc.dg/tree-ssa/slsr-40.c: New test.