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