On Thu, 4 Sep 2014, Jeff Law wrote: > On 09/04/14 06:11, Richard Biener wrote: > > > > The following two patches each fix PR63148, a wrong-code issue > > caused by bogus array indices a-la > > &global_data.b[(sizetype) i + 536870911] which have a correct > > address when lowered but bogus index. > > > > The case in question can be mitigated by disabling folding of > > (sizetype) i * 8 + 4294967288 to ((sizetype) i + 536870911) * 8 > > but that makes the real error - the existence of > > try_move_mult_to_index - just harder to trigger (I have already > > removed all similar code over the years). > > > > So the second variant is removing try_move_mult_to_index. > > > > The wrong-code is a regression from 4.7 which means a fix > > for the branches would be nice as well. > > > > Removing try_move_mult_to_index makes us correctly detect > > the dependence and not vectorize the testcase while only > > doing the mitigation makes dependence analysis fail and > > we create a runtime alias check (which would be a regression > > as well here - 4.7 also didn't vectorize this by detecting > > the dependence). > > > > I'm not sure which patch has the least side-effects on the > > branches. > > > > Sofar I have only fully tested removing try_move_mult_to_index > > on trunk which has some fallout that I have fixed and some > > fallout that should be addressed as followup. The patch > > contains three new testcases from PR19807 we didn't add > > and for which we regressed two with 4.8 already (-2 and -3) > > and they still fail after the patch. > > > > Patch 2 is bootstrapped and tested on x86_64-unknown-linux-gnu, > > patch 1 is in testing.
I have now tested patch 1 dumbing down fold_plusminus_mult_expr and it passes bootstrap and testing without further fallout. So even as it isn't a real fix for the issue I suspect it may have less side-effects than the other patch ripping out try_move_mult_to_index completely. So I am considering patch 1 for the branches (but I'm yet unsure about it). Having to maintain two different states of the fix also doesn't sound too great. > > I have a strong preference to have patch 2 on trunk. > > > > Any preferences for the branches? > > > > Thanks, > > Richard. > > > > > > 2014-09-04 Richard Biener <rguent...@suse.de> > > > > PR middle-end/63148 > > * fold-const.c (fold_plusminus_mult_expr): Avoid case > > producing invalid array indexes when the multiplication > > is stripped. > > > > * gcc.dg/vect/pr63148.c: New testcase. > Given the pain we've had through the years with these kinds of out-of-bounds > indices which are compensated for elsewhere in the address computation, I'm > all for making them go away. > > As two examples of where this matters, the PA port does its segment selection > on just the base register, not the entire effective address. Thus when we muck > around with indices like this, the base register has the compensation code and > thus may result in selecting a different segment than would be the case if we > looked at the entire effective address. > > One of the mn103 parts has a similar problem, but it was actually a processor > bug -- it was supposed to work correctly and select from the entire effective > address, but it was broken in the hardware and we've had the "joys" of working > around that errata in the compiler. > > The argument for keeping this kind of braindamage has that it's allowed for > more efficient code for things like virtual origin array addressing in Ada. > But it's been nothing but pain. Ok, so for now I am going to apply the removal of try_move_mult_to_index to trunk and see whether anything bad falls out of that. Richard.
