[PATCH] D81756: [SCCP] Turn sext into zext for positive ranges.

[Eli Friedman via Phabricator via llvm-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

LGTM



================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1311
             IV, &CB,
-            ValueLatticeElement::getRange(NewCR, /*MayIncludeUndef=*/true));
+            ValueLatticeElement::getRange(NewCR, /*MayIncludeUndef=*/false));
         return;
----------------
fhahn wrote:
> efriedma wrote:
> > I'm a little uncomfortable that flipping this from true to false impacted zero testcases...
> > 
> > I'm not sure how to judge exactly how risky it is to land this without fixes to various passes we know have issues.  I'm particularly worried about loop unswitch, since we saw a real miscompile involving it in the past.
> > I'm a little uncomfortable that flipping this from true to false impacted zero testcases...
> 
> > I'm not sure how to judge exactly how risky it is to land this without fixes to various passes we know have issues. I'm particularly worried about loop unswitch, since we saw a real miscompile involving it in the past.
> 
> Yeah, it's probably best to wait at least until the next release. I'll mention it in https://bugs.llvm.org/show_bug.cgi?id=46144. For now, I removed the change to MayIncludeUndef. That limits the cases we can catch, but there still are some.
> 
> I think the reason for why there are no changes to other test cases is because we currently only use ranges to replace compares with constants. When replacing with a constant I think we concluded that it does not matter whether the ranges include undef or not.
> 
> When it comes to replacing with other instructions based on range info like in this patch, we need to check for undef I think.
> 
> 
> 
> I think the reason for why there are no changes to other test cases is because we currently only use ranges to replace compares with constants. 

Oh, right.


================
Comment at: llvm/lib/Transforms/Scalar/SCCP.cpp:1635
+        Inst.replaceAllUsesWith(ZExt);
+        Inst.eraseFromParent();
+        InstReplacedStat++;
----------------
fhahn wrote:
> efriedma wrote:
> > Are we keeping around stale pointers in the Solver?  That's technically UB, I think, even if the code works otherwise, and the compiler is unlikely to take advantage.
> Yes it does currently. I don't think it would cause in problems in practice, but it is better to properly remove it. I've added a `removeLatticeValueFor` and used that. We don't remove the entries when removing other instructions, but I guess if we create new instructions we could in theory re-use existing pointers or something.
Reusing existing pointers would only be if we tried to query the solver for an instruction we created after the solver runs; I don't think we do that.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D81756/new/

https://reviews.llvm.org/D81756