-
Notifications
You must be signed in to change notification settings - Fork 112
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
base: main
Are you sure you want to change the base?
Conversation
There was a problem hiding this 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
/// 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); | ||
} |
There was a problem hiding this comment.
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.
} | ||
|
||
/// True iff all results differentially depend on all operands | ||
// TODO: differential dependency/activity interface |
There was a problem hiding this comment.
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) { |
There was a problem hiding this 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?
b40b4d1
to
2e0a794
Compare
@pengmai I'm going to merge for now to avoid rebase issues, but comments above should be addressed subsequently |
2e0a794
to
d036184
Compare
d036184
to
63c2079
Compare
No description provided.