[PATCH] D37227: [SCEV] Ensure ScalarEvolution::createAddRecFromPHIWithCastsImpl is working with an add expression.

[Daniel Neilson via Phabricator via llvm-commits]

dneilson added inline comments.


================
Comment at: lib/Analysis/ScalarEvolution.cpp:4387
+
   const SCEV *BEValue = getSCEV(BEValueV);
 
----------------
reames wrote:
> I think you've misdiagnosed the bug here.  getSCEV is supposed to always return a SCEV for a value when requested.  It may return SCEVUnknown, but that's still a valid SCEV.  As such, the check for AddRec just below this should prevent the xor flowing through.  I suspect you're actual issue is that getSCEV has return nullptr.
> 
> p.s. I suggest chatting with Max or Sanjoy to confirm everything I just said.  I'm not SCEV expert.
That doesn't mesh with what I was seeing here. What I was seeing with the added test case is that we get all the way down to line 4479, and blow up there. PHISCEV appears to be a constant value of -7 rather than any sort of AddRec, so the cast on that line ends up failing. 

When I dump the intermediate values I find that BEValueV is the xor in the loop (%tmp4 = xor i64 %tmp3, -9223372036854775808). This method seems to be written assuming that BEValueV is an add operator -- the RHS is even cast to a SCEVAddExpr on line 4392. It seems pretty clear that an SCEVAddRec is for the addition operator, and constructing one from any other operator is just asking for trouble; hence the patch as written...

Also, the if directly before this one returns 'None' to signify an early bail-out, so it seems to me that's the expected behaviour of this method.


https://reviews.llvm.org/D37227