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.

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

[mlir] Summary-based activity analysis #2114

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Conversation

pengmai
Copy link
Member

@pengmai pengmai commented Oct 9, 2024

No description provided.

@pengmai pengmai requested review from wsmoses and ftynse October 9, 2024 16:08
@pengmai pengmai marked this pull request as ready for review October 9, 2024 16:11
Copy link
Collaborator

@ftynse ftynse left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks reasonable overall, please give @wsmoses a chance to take a look

enzyme/Enzyme/MLIR/Analysis/ActivityAnnotations.cpp Outdated Show resolved Hide resolved
Comment on lines +74 to +79
/// True iff all results differentially depend on all operands
// TODO: differential dependency/activity interface
// TODO: Select cond is not fully active
static bool isFullyActive(Operation *op) {
return isa<LLVM::FMulOp, LLVM::FAddOp, LLVM::FDivOp, LLVM::FSubOp,
LLVM::FNegOp, LLVM::FAbsOp, LLVM::SqrtOp, LLVM::SinOp, LLVM::CosOp,
LLVM::Exp2Op, LLVM::ExpOp, LLVM::LogOp, LLVM::InsertValueOp,
LLVM::ExtractValueOp, LLVM::BitcastOp, LLVM::SelectOp>(op);
}

static bool isNoOp(Operation *op) {
return isa<LLVM::NoAliasScopeDeclOp, LLVM::LifetimeStartOp,
LLVM::LifetimeEndOp, LLVM::AssumeOp>(op);
}
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suspect we have a copy of this elsewhere and would rather have one definition that is used in both, e.g., by having a static inline function in a header. Otherwise we run the risk of forgetting to update one of the copies and wondering why it doesn't work.

enzyme/Enzyme/MLIR/Analysis/ActivityAnnotations.cpp Outdated Show resolved Hide resolved
enzyme/Enzyme/MLIR/Analysis/ActivityAnnotations.cpp Outdated Show resolved Hide resolved
enzyme/Enzyme/MLIR/Analysis/ActivityAnnotations.cpp Outdated Show resolved Hide resolved
}

/// True iff all results differentially depend on all operands
// TODO: differential dependency/activity interface
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

So we do now have such an interface:

static SmallVector<Value> getPotentialIncomingValues(OpResult res) {

Copy link
Member

@wsmoses wsmoses left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could a relevant test be added to make sure it doesn’t break?

@pengmai pengmai force-pushed the jmp/summary-activity branch 3 times, most recently from b40b4d1 to 2e0a794 Compare October 11, 2024 21:39
@wsmoses
Copy link
Member

wsmoses commented Oct 14, 2024

@pengmai I'm going to merge for now to avoid rebase issues, but comments above should be addressed subsequently

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants