This is an archive of the discontinued LLVM Phabricator instance.

[Attributor][NFC] Connect AAPotentialValues with AAValueSimplify
ClosedPublic

Authored by okura on Aug 10 2020, 10:41 AM.

Details

Summary

This patch enables AAValueSimplify to use information from AAPotentialValues

Diff Detail

Event Timeline

okura created this revision.Aug 10 2020, 10:41 AM
Herald added a reviewer: homerdin. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
okura requested review of this revision.Aug 10 2020, 10:41 AM
jdoerfert added inline comments.Aug 10 2020, 10:55 AM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
4489

I'm not sure we need/want this failAll flag. Can't we just return true, given that ::None is the right simplified value in that case? With the current setup we will always call AAPotentialValues, even if the range analysis would determine a constant result eventually.

okura added inline comments.Aug 10 2020, 6:21 PM
llvm/lib/Transforms/IPO/AttributorAttributes.cpp
4489

Can't we just return true, given that ::None is the right simplified value in that case?

I thought llvm::None is a weaker result than concrete ConstantInt values, but that was my mistake. I'll fix this.

okura updated this revision to Diff 284542.Aug 10 2020, 6:25 PM
  • fix the places that were pointed out
jdoerfert accepted this revision.Aug 10 2020, 7:07 PM

LGTM, some minor nits to consider.

llvm/lib/Transforms/IPO/AttributorAttributes.cpp
4484

Nit: Even better if we add a dependence only if we use the result, in either case, OPTIONAL is the right kind :)

4498

You don't need an else after return, that should bring all the indention to a single level here.

4504

early exits:

if (ask...)
  return true;
if (ask...)
  return true;
return false;
This revision is now accepted and ready to land.Aug 10 2020, 7:07 PM
okura updated this revision to Diff 284584.Aug 10 2020, 11:38 PM
  • fix nits
  • modify tests
  • move changes in PotentialValuesState from D85632

I'm sorry for forgetting to include changes in tests. There are changes in the number of iterations only.
And I moved PotentialValuesState's change to this because the bug in there causes a wrong value simplification.