Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

[mlir] don't incorrectly bail from alias analysis #1570

Merged
merged 4 commits into from
Jan 5, 2024
Merged

[mlir] don't incorrectly bail from alias analysis #1570

merged 4 commits into from
Jan 5, 2024

Conversation

ftynse
Copy link
Collaborator

@ftynse ftynse commented Dec 7, 2023 •
edited
Loading

  • Alias transfer behavior isn't always completely described by memory
    effects, be conservative about that.
  • "Global" effects with no target value should not be ignored.
  • Interpret readnone as "not reading or writing" rather than just "not
    reading", in line with LLVM.
  • Simplify code in external callee attribute-based transfer function.
  • Treat all stores as "may" because to meet the expectations of the
    activity post-analysis that only looks at function exit points and
    therefore needs conservative results.

@ftynse ftynse requested review from wsmoses and pengmai December 7, 2023 13:07
@ftynse ftynse mentioned this pull request Dec 7, 2023
Merged
Base automatically changed from ftynse/unfixed to main January 2, 2024 15:42
ftynse added 2 commits January 2, 2024 16:38
@ftynse
[mlir] clean up alias analysis
b36a0e7 
* Interpret `readnone` as "not reading or writing" rather than just "not
  reading", in line with LLVM.
* Simplify code in external callee attribute-based transfer function.
* Treat all stores as "may" because to meet the expectations of the
  activity post-analysis that only looks at function exit points and
  therefore needs conservative results.
@ftynse
[mlir] don't incorrectly bail from AliasAnalysis::transfer
8e037ef 
* Alias transfer behavior isn't always completely described by memory
  effects, be conservative about that.
* "Global" effects with no target value should not be ignored.
@ftynse ftynse force-pushed the ftynse/alias-cleanup branch from 41ca1e0 to 8e037ef Compare January 2, 2024 16:38
@ftynse ftynse marked this pull request as ready for review January 2, 2024 16:38
@wsmoses
Copy link
Member

wsmoses commented Jan 4, 2024

@ftynse CI is now running and the CI failures are actually real from the PR:

********************
Testing:  0.. 10.. 20.. 30.. 40.. 50.. 60.. 70.. 80.. 90.. 
********************
Failed Tests (10):
  Enzyme :: MLIR/ActivityAnalysis/activereturn.mlir
  Enzyme :: MLIR/ActivityAnalysis/allocator.mlir
  Enzyme :: MLIR/ActivityAnalysis/allocret.mlir
  Enzyme :: MLIR/ActivityAnalysis/callgraph.mlir
  Enzyme :: MLIR/ActivityAnalysis/memalgn.mlir
  Enzyme :: MLIR/ActivityAnalysis/multiptr.mlir
  Enzyme :: MLIR/ActivityAnalysis/string.mlir
  Enzyme :: MLIR/ActivityAnalysis/subld.mlir
  Enzyme :: MLIR/AliasAnalysis/escaping_memory.mlir
  Enzyme :: MLIR/AliasAnalysis/llvm_casts.mlir

@wsmoses
Copy link
Member

wsmoses commented Jan 5, 2024

Hm looks like those errors already exist in main, regardless

@wsmoses wsmoses merged commit f928881 into main Jan 5, 2024
43 of 58 checks passed
@wsmoses wsmoses deleted the ftynse/alias-cleanup branch January 5, 2024 04:32
MilesCranmer pushed a commit to MilesCranmer/Enzyme that referenced this pull request Jul 24, 2024
@wsmoses
uparm (EnzymeAD#1570)
d4f44d6
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@wsmoses wsmoses wsmoses approved these changes

@pengmai pengmai Awaiting requested review from pengmai

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@ftynse @wsmoses