[PATCH] D41578: [SCEV] Do not cache S -> V if S is not equivalent of V

[Serguei Katkov via Phabricator via llvm-commits]

skatkov added a comment.

In https://reviews.llvm.org/D41578#980133, @eastig wrote:

> In https://reviews.llvm.org/D41578#979947, @skatkov wrote:
>
> > > For "%mul86.i = mul nuw nsw i32 %p280, %p281" isSCEVExprNeverPoison return false which causes the SCEV '(%p280 * %p281)' to have SCEV::FlagAnyWrap instead of a combination of  SCEV::FlagNUW and Flags, SCEV::FlagNSW.
> > > 
> > > Am I correct SCEV::FlagAnyWrap means no poisoned values? If so, this looks strange. Shouldn't it be:
> >
> > As I understand this flag means have no idea, in other words may overflow.
>
>
> When SCEV::FlagAnyWrap is set SCEVNAryExpr::hasNoUnsignedWrap and SCEVNAryExpr::hasNoSignedWrap return false:
>
>   bool hasNoUnsignedWrap() const {
>     return getNoWrapFlags(FlagNUW) != FlagAnyWrap;
>   }
>   
>   bool hasNoSignedWrap() const {
>     return getNoWrapFlags(FlagNSW) != FlagAnyWrap;
>   }
>   
>
> From this I can conclude SCEV::FlagAnyWrap mean no poisoned values.
>
> >> 
> >> 
> >>   return isSCEVExprNeverPoison(BinOp) ? SCEV::FlagAnyWrap : Flags;
> > 
> > No, I read this code as if isSCEVExprNeverPoison(BinOp) is true then we can trust the flags otherwise we cannot...
> > 
> > But it would be better to hear Sanjoy's opinion here.
>
> Sergey, it looks like your patch does not take into account that nsw and nuw have not been set because of additional analysis, e.g. isSCEVExprNeverPoison. So SCEVLostPoisonFlags thinks flags are lost but they were not set intentionally. Where the issue your patch fixes come from? I don't see any IR test.


I understand what you mean, however the patch does a right thing in general. SCEV without nuw/nsw should not correspond to instruction within these flags.
The functional bug I had looked as follows:
there were two instructions, say:
%a = add nuw nsw %c, -1
%b = add %c, -1

First for instruction %a the SCEV S has been built.
Then for instruction % SCEV S' has been build. Due to loosing the nuw/nsw flags S == S'.
Later SCEV expander has been used to expand S'. But as soon as we mapped S'==S => %a, %a has been used as expansion of S'.
So in reality it resulted in %s with nuw/nsw has been used where %b should be used and it introduces usage of nuw/nsw instruction where it was not expected.
The later backend did a wrong code generation for -1 due to present of nuw/nsw flags in an instruction.

Let me try to add a clearance of nuw/nsw flags in expander as I explained before...


Repository:
  rL LLVM

https://reviews.llvm.org/D41578