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. Thanks, Bill > > Thanks, > Bill > > On Tue, 2013-10-01 at 16:06 +0100, Yufeng Zhang wrote: > > Hi Bill, > > > > Thank you for the review and the offer to help. > > > > On 10/01/13 15:36, Bill Schmidt wrote: > > > On Tue, 2013-10-01 at 08:17 -0500, Bill Schmidt wrote: > > >> On Tue, 2013-10-01 at 12:19 +0200, Richard Biener wrote: > > >>> On Wed, Sep 25, 2013 at 1:37 PM, Yufeng Zhang<yufeng.zh...@arm.com> > > >>> wrote: > > >>>> Hello, > > >>>> > > >>>> Please find the updated version of the patch in the attachment. It has > > >>>> addressed the previous comments and also included some changes in > > >>>> order to > > >>>> pass the bootstrapping on x86_64. > > >>>> > > >>>> It's also passed the regtest on arm-none-eabi and aarch64-none-elf. > > >>>> > > >>>> It will also fix the test failure as reported here: > > >>>> http://gcc.gnu.org/ml/gcc-patches/2013-09/msg01317.html > > >>>> > > >>>> OK for the trunk? > > >>> > > >>> + where n is a 32-bit unsigned int and pointer are 64-bit long. In > > >>> this > > >>> + case, the gimple for (n - 1) is: > > >>> + > > >>> + _2 = n_1(D) + 4294967295; // 0xFFFFFFFF > > >>> + > > >>> + and it is wrong to multiply the large constant by 4 in the 64-bit > > >>> space. */ > > >>> + > > >>> +static bool > > >>> +safe_to_multiply_p (tree type, double_int cst) > > >>> +{ > > >>> + if (TYPE_UNSIGNED (type) > > >>> +&& ! double_int_fits_to_tree_p (signed_type_for (type), cst)) > > >>> + return false; > > >>> + > > >>> + return true; > > >>> +} > > >>> > > >>> This looks wrong. The only relevant check is as whether the > > >>> multiplication overflows the original type as you miss the implicit > > >>> truncation that happens. Which is something you don't know > > >>> unless you know the value. It definitely isn't a property of a type > > >>> and a constant but the property of two constants and a type. > > >>> Or the predicate has a wrong name. > > >>> > > >>> The use of get_unwidened in this core routine looks like this is > > >>> all happening in the wrong place and we should have picked up > > >>> another candidate for this instead? I'm sure Bill will know more here. > > >> > > >> I'm not happy with how this patch is progressing. Without having looked > > >> too deeply, this might be better handled earlier when determining which > > >> casts are safe to use in building candidates. What you have here seems > > >> more like closing the barn door after the horse got out. Maybe that's > > >> the only solution, but it doesn't seem likely. > > >> > > >> Another problem is that your test case isn't testing anything except > > >> that the compiler doesn't crash. That isn't sufficient as a regression > > >> test. > > >> > > >> I'll spend some time looking at this to see if I can find a better > > >> approach. It might be a day or two before I can get to it. In addition > > >> to the included test case, are there any other cases you've found that I > > >> should be concerned with? > > > > > > To help me investigate this without having to build a cross compiler, > > > could you please compile your test case (without the patch applied) > > > using -fdump-tree-reassoc2 -fdump-tree-slsr-details and send me the > > > generated dump files? > > > > The issue is not specific to AArch64; please find the attached dumps > > generated from the x86-64 gcc by compiling gcc.dg/tree-ssa/slsr-39.c. > > > > W.r.t your comment in the other email about adding a test to verify the > > expected gimple, I think the existing test gcc.dg/tree-ssa/slsr-39.c is > > sufficient. The test currently fails on both AArch64 and x86-64, and > > presumably also fails on any other 64-bit target where pointer is 64-bit > > and int is 32-bit size. The patch I proposed is to fix this issue and > > gcc.dg/tree-ssa/slsr-39.c itself shall be a good regression test (with > > specific verification on gimple ir). > > > > The new test proposed in this patch is to regtest the issue my original > > patch has, which is a runtime failure due to incorrect optimization. > > > > I'll address other comments in separate emails. > > > > Thanks, > > Yufeng
