-
Notifications
You must be signed in to change notification settings - Fork 17
Align the bufferization pipeline according to the upstream doc #295
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
Conversation
bufferization::OneShotBufferizationOptions options; | ||
options.bufferizeFunctionBoundaries = true; | ||
options.setFunctionBoundaryTypeConversion( | ||
bufferization::LayoutMapOption::IdentityLayoutMap); | ||
pm.addPass(bufferization::createOneShotBufferizePass(options)); | ||
pm.addPass(createCanonicalizerPass()); | ||
pm.addPass(createCSEPass()); | ||
pm.addPass(createCanonicalizerPass()); |
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.
L101 is already CanonicalizerPass, do we really need to do it again after CSE?
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.
Yes, the first CanonicalizerPass
is mainly used to canonicalize the scf.for
and scf.forall
to the canonical form in memref, exposing more CSE
optimization opportunities. The CSE
pass is used to eliminate the common memref.subview
so there will be some useless memref.copy memref.copy %a, %a
. The last CanonicalizerPass
is used to eliminate the useless memref.copy
which copies from A to A like memref.copy %a, %a
lib/gc/Transforms/Pipeline.cpp
Outdated
pm.addPass(bufferization::createDropEquivalentBufferResultsPass()); | ||
pm.addNestedPass<func::FuncOp>( | ||
bufferization::createPromoteBuffersToStackPass()); | ||
mlir::bufferization::BufferDeallocationPipelineOptions deallocOption; |
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.
nit: no need to add mlir
namespace here.
BTW, could you also put a note/link to the upstream bufferization pipeline?
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.
Thanks for the advice! the mlir namespace has been removed and the note has been added
Fix part of issue #288
empty_tensor_elimination
could eliminate the copy introduced bybufferization.materialize_in_destination
. And adjust the pipeline according to the doc(https://mlir.llvm.org/docs/Bufferization/#overview).