[PATCH] D61314: [SCCP] Remove forcedconstant, use InstSimplify to fold undef values.

Sun Feb 9 12:06:19 PST 2020

fhahn updated this revision to Diff 243456.
fhahn added a comment.

Removed simplifications based on undef, just go to overdefined directly.

In D61314#1864913 <https://reviews.llvm.org/D61314#1864913>, @efriedma wrote:

> I'd prefer to get rid of ResolveUndefsIn completely, if possible.  If we don't special-case UndefValue anywhere in SCCP, that should just work, I think?  SCCP should still be correct even though UndefValue isn't a "normal" constant; it doesn't try to make assumptions based on the pointer equivalence of two "constant" values, except for joins like PHI nodes where it shouldn't matter. Am I missing something?  How much optimization power do we lose that way, versus this patch's approach?

> The approach here is a little different from that.  I'm understanding it correctly, the idea is that in cases where we can't prove a forced constant is actually constant, we instead go straight to overdefined?  So we sort of keep our "undef=>undefined" extension of the SCCP model, but get rid of the shakiest part of it?  That should work, I think... but I'm not sure the whole ResolvedUndefsIn is giving you any significant benefit at that point.  At least, I can't think of a case off the top of my head where it helps.  Maybe I'm forgetting something.

I think this and the first paragraph are addressed in the comments later on.

> I'm not completely confident calling Simplify* with operands that aren't Constants does the right thing here; that's sort of an extension to the algorithm.  I guess it should work, though?

The calls to Simplify*  mostly took care of folding existing undef constants. There are a few tests that check for that, but in practice it seems to not really pop up, as other passes should clean up those. I've also compared the stats to the previous version of the patch, and there are no changes.

> Does this patch in its current form still depend on any of the other patches?

No, it does not. I've removed the dependencies.

> Oh, hmm, in terms of optimization I guess you get some benefit from making assumptions about the behavior of undef, instead of only interacting with it through constant folding.  You get better results by implicitly unifying UndefValue with other constants in PHI nodes and call arguments, and forcing branches one way instead of both ways.  Maybe this is better than completely dropping ResolveUndefsIn.  Maybe this approach is fine, then.

Yes I think there is a benefit of delaying resolving undefs till we finished with an iteration, as we might discover a concrete value later, e.g. for PHIs where we discover that some predecessors are executable later in the iteration. As is I think the patch removes all forced-constant related machinery, which should be the most problematic part of ResolvedUndefsIn.

> I'd prefer to finish and land this first, and let it bake a little while, before landing the other pieces.  It seems a little riskier than the rest, since it's really hard to reason about undef-related changes.

Sounds good! What do you think of the latest version of the patch? The remainder of ResolvedUndefsIn deals with undef constants as branch conditions. I think keeping those should be save, as they do not really interact with the lattice values.

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D61314

Files:
  llvm/lib/Transforms/Scalar/SCCP.cpp
  llvm/test/Transforms/IPConstantProp/PR16052.ll
  llvm/test/Transforms/IPConstantProp/PR26044.ll
  llvm/test/Transforms/SCCP/2006-12-19-UndefBug.ll
  llvm/test/Transforms/SCCP/apint-bigint2.ll
  llvm/test/Transforms/SCCP/apint-ipsccp3.ll
  llvm/test/Transforms/SCCP/apint-select.ll
  llvm/test/Transforms/SCCP/ip-constant-ranges.ll
  llvm/test/Transforms/SCCP/ipsccp-basic.ll
  llvm/test/Transforms/SCCP/logical-nuke.ll
  llvm/test/Transforms/SCCP/switch-multiple-undef.ll
  llvm/test/Transforms/SCCP/ub-shift.ll
  llvm/test/Transforms/SCCP/undef-resolve.ll

-------------- next part --------------
A non-text attachment was scrubbed...
Name: D61314.243456.patch
Type: text/x-patch
Size: 37996 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200209/624cc3a0/attachment.bin>