feat(DeadCodeElimination - While loop and Function wrapper): Add WhileStatementElimination and UnusedFunctionElimination in DCE pass#39
Conversation
Introduces a new WhileStatementElimination pass to the Dead Code Elimination framework. This pass identifies 'while' loops with a constantly false condition and removes them entirely. Also includes minor header reordering and adjusts pointer syntax for consistency within 'pass.cc'.
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
shanjiang98
changed the title
maldoca/js/ir/transforms/dead_code_elimination/tests/if_statement/BUILD
Outdated
Show resolved
Hide resolved
maldoca/js/ir/transforms/dead_code_elimination/tests/unused_functions/input.js
Outdated
Show resolved
Hide resolved
maldoca/js/ir/transforms/dead_code_elimination/tests/while_statement/input.js
Outdated
Show resolved
Hide resolved
maldoca/js/ir/transforms/dead_code_elimination/tests/while_statement/input.js
Show resolved
Hide resolved
| mlir::Operation *current_op = def_op->getParentOp(); | ||
| while (current_op != nullptr) |
There was a problem hiding this comment.
for (mlir::Operation *current_op = def_op->getParentOp();
current_op != nullptr;
current_op = current_op->getParentOp()) {
if (current_trivia == nullptr) {
continue;
}
...
}
- Updated BUILD file to correct the path for lit test files. - Simplified the passes in the README and run generated files by removing unnecessary passes. - Adjusted indentation in input.js for better readability. - Cleaned up output.generated.txt by removing excessive comments and irrelevant sections.
| struct SymbolInfo { | ||
| std::vector<mlir::Operation*> definitions; | ||
| std::vector<mlir::Operation*> references; | ||
| // Symbols defined within the scope of this symbol's definition (e.g., if |
There was a problem hiding this comment.
Is this vector computed recursively? Or does it only include the variables one level deeper?
For example, the current symbol f is a function. Within f there's an inner function g, which contains yet another inner function h. Does this vector include h or only g?
| absl::flat_hash_map<JsSymbolId, SymbolInfo> symbol_infos; | ||
|
|
||
| root_op->walk([&](mlir::Operation* op) { | ||
| // TODO: We can remove this special handling once the trivia is fixed. |
There was a problem hiding this comment.
Can you mention the issue number in the comment?
// TODO(#xx)
|
|
||
| for (auto& [symbol, info] : symbol_infos) { | ||
| if (info.definitions.empty()) { | ||
| continue; |
There was a problem hiding this comment.
Can you check this scenario:
let a = 1;
b = a;
let c = b;I remember that JavaScript spec says b is treated as a global variable. This also requires non-strict mode during parsing.
| current_op != nullptr; current_op = current_op->getParentOp()) { | ||
| auto current_trivia = | ||
| llvm::dyn_cast<JsirTriviaAttr>(current_op->getLoc()); | ||
| if (current_trivia != nullptr) { |
There was a problem hiding this comment.
if (current_trivia == nullptr) {
continue;
}| } | ||
|
|
||
| // Check if info.definitions contains duplicates across symbol_infos? If so, | ||
| // print warning |
There was a problem hiding this comment.
We need to make this comment consistent with the code. If you don't want spamming the log, you can use CHECK(...).
| } | ||
|
|
||
| for (auto& [symbol, info] : symbol_infos) { | ||
| if (info.references.empty()) { |
There was a problem hiding this comment.
// TODO: Also consider if the symbol is written to.
| // When we remove a function declaration, we are also removing all ops | ||
| // in the function body, including definitions of local variables. | ||
| // This means we need to remove those SymbolInfo records. | ||
| std::vector<JsSymbolId> worklist = info.inner_definitions; |
There was a problem hiding this comment.
This looks like a stack instead of a queue - does this matter?
|
|
||
| auto it = symbol_infos.find(current_symbol_id); | ||
| if (it != symbol_infos.end()) { | ||
| it->second.definitions.clear(); |
There was a problem hiding this comment.
Is this unnecessary? We are erasing it anyway.
| symbol_infos.erase(it); | ||
| } | ||
| } | ||
| def_op->erase(); |
There was a problem hiding this comment.
It seems that we should also remove the symbol info entry for def_op itself - we are already removing the symbol infos for all its inner symbols, but not itself.
You might need to change the for (auto& [symbol, info] : symbol_infos) { into something like:
for (auto it = symbol_infos.begin(); it != symbol_infos.end(); ??) {
...
if `it` should be erased:
it = symbol_infos.erase(it);
if `it` should not be erased:
it++;
}| auto outer_it = symbol_infos.find(outer_symbol); | ||
| if (outer_it != symbol_infos.end()) { | ||
| auto& outer_info = outer_it->second; | ||
| outer_info.inner_definitions.erase( |
There was a problem hiding this comment.
Optional: You can check if inner_definitions can be a set instead of a vector.
If you want to keep insertion order, use llvm::SetVector.
2 participants
Introduces a new WhileStatementElimination pass to the Dead Code Elimination framework. This pass identifies 'while' loops with a constantly false condition and removes them entirely.