[mlir] Add Normalize pass #162266
[mlir] Add Normalize pass #162266
Conversation
|
✅ With the latest revision this PR passed the C/C++ code formatter. |
Sh0g0-1758
force-pushed
the
mlir-canon
branch
from
d14a41f to
0836dad
Compare
| def Normalize : Pass<"normalize", "ModuleOp"> { | ||
| let summary = "normalize."; | ||
| let description = [{ | ||
| just normalize bro |
| "func::FuncDialect", | ||
| "scf::SCFDialect", | ||
| "vector::VectorDialect", | ||
| "LLVM::LLVMDialect", |
There was a problem hiding this comment.
Why are these needed as dependent dialects?
|
|
||
| bool isInitialOperation(mlir::Operation *const op) const noexcept; | ||
| void nameAsInitialOperation( | ||
| mlir::Operation *op, |
There was a problem hiding this comment.
Don't need mlir:: as you have using mlir namespace above.
| const uint64_t MagicHashConstant = 0x6acaa36bef8325c5ULL; | ||
| void | ||
| collectOutputOperations(Block &block, | ||
| SmallVector<Operation *, 16> &Output) const noexcept; |
There was a problem hiding this comment.
MLIR has style guide around variable names https://mlir.llvm.org/getting_started/DeveloperGuide/#style-guide
|
|
||
| uint64_t Hash = MagicHashConstant; | ||
|
|
||
| uint64_t opcodeHash = strHash(op->getName().getStringRef().str()); |
There was a problem hiding this comment.
StringRef has hash_value , could that be used?
| @@ -0,0 +1,436 @@ | |||
| //===- Normalize.cpp - Conversion from MLIR to its canonical form ---------===// | |||
There was a problem hiding this comment.
Why not make this pass local to a tool?
| if (op.hasTrait<OpTrait::IsTerminator>()) | ||
| return true; | ||
|
|
||
| if (auto memOp = dyn_cast<MemoryEffectOpInterface>(&op)) { |
| Operands.push_back({Stream.str(), operand}); | ||
| } | ||
|
|
||
| if (op->hasTrait<OpTrait::IsCommutative>()) { |
There was a problem hiding this comment.
commutative operations are naturally reordered while considering constants, would this be considered here too giving naming conventions?
| std::equal(check.begin(), check.end(), base.begin()); | ||
| } | ||
|
|
||
| void NormalizePass::foldOperation(mlir::Operation *op) { |
There was a problem hiding this comment.
In MLIR there is OpFolder and folding, I think this is doing something different, could you document it?
| Name.append(opName.substr(1, 7)); | ||
|
|
||
| Name.append("$"); | ||
| for (unsigned long i = 0; i < Operands.size(); ++i) { |
There was a problem hiding this comment.
https://llvm.org/docs/CodingStandards.html#don-t-evaluate-end-every-time-through-a-loop , although in this case the range based should be easy to use and preferred.
|
|
||
| // CHECK-LABEL: module { | ||
| // CHECK: func.func @infinte_loop(%[[ARG0:.*]]: memref<?xi32>, %[[ARG1:.*]]: i32) { | ||
| // CHECK: %vl15969$e5677$ = arith.constant 1 : i32 |
There was a problem hiding this comment.
What's up with these cryptic SSA variable names?
There was a problem hiding this comment.
That's the point of llvm-canon, it gives everything predictable "canonical" names so when you compare two files textually, they don't differ because of SSA value numbers.
| } | ||
|
|
||
| uint64_t inline strHash(std::string_view data) noexcept { | ||
| const static uint64_t FNV_OFFSET = 0xcbf29ce484222325ULL; |
There was a problem hiding this comment.
What is the meaning of these magic numbers?
| private: | ||
| const uint64_t MagicHashConstant = 0x6acaa36bef8325c5ULL; | ||
| void | ||
| collectOutputOperations(Block &block, |
There was a problem hiding this comment.
All of the functions here should have documentation.
There was a problem hiding this comment.
Thanks for your patch! Please provide a clear commit description, explaining why you make changes, not only what they are: https://mlir.llvm.org/getting_started/Contributing/#commit-messages.
Two large items that will need addressing is (1) this does not belong to lib/Conversion because it is not a conversion and it will have to operate generally on all MLIR operations regardless of dialects and (2) we need to find a way of testing the desired goal of the tool which is minizing textual difference between files to "semantically meaningful" parts.
5 participants
PR to add an llvm-canon kind of tool for MLIR.