Re: [PATCH] Fix PR63148 - two choices

[Richard Biener <rguenther at suse dot de>]

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