-
Notifications
You must be signed in to change notification settings - Fork 532
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
add argmax lowering and integer return types #299
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.
You are the first to step on the landmine of integer dtypes in TorchToLinalg :) You will need to add a case for it in setupValueTensorToBuiltinTensorConversion which transitively calls into ValueTensorType::toBuiltinTensor -- that function needs to be updated to translate the dtypes to builtin integer types. Specifically, float types get passed through, and integer types get converted to a signless integer version of the same bitwidth.
b699113
to
a2c96d2
Compare
a2c96d2
to
8f06c4e
Compare
8f06c4e
to
4525b7b
Compare
5501ae6
to
0038b4f
Compare
0038b4f
to
b6ed300
Compare
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 good! Just need to delete a few lines of redundant code.
b6ed300
to
6114dea
Compare
6114dea
to
578c4f4
Compare
else | ||
knowledge.sizes.erase(knowledge.sizes.begin() + dim); | ||
} | ||
} else { |
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 just realize similar to the OpConversionPattern
we can't assume else block is always None
. It could be variable. For integer variable, we can't known statically the sizes of each dim so use:
else if (op.dim().getType().isa<IntegerType>())
knowledge.sizes.resize(keepdimBool ? inputRank : inputRank - 1,
kUnknownSize);
For NoneType case, nothing is needed as knowledge.hasSizes = true;
is set above already. It's nice to keep comment about None case though.
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.
Sorry, I should have caught this after your other comment. Thank you for the very thorough review
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.
@dan-garvey Hey Daniel, sry that the op.dim().getType().isa<IntegerType>()
above was wrong it should be Torch::IntType
. We need to check against torch type. I will fix this as part of my PR.
578c4f4
to
6fbbd9f
Compare
Add argmax lowering from torch to linalg
6fbbd9f
to
8b18515
Compare
* add shape inference * Revert "add shape inference" This reverts commit f9d42f39e68e14b5648abccfc8617fff00244d16. * shape inference * test case * format
* NFC: Attribute cleanup (remove references of attributes) (llvm#286) * Define krnl.permute op. * Support krnl.permute operation. * Properly remove loop references. * Re-push, Github was down. * Need to debug interpretOp error. * Fix lowering bug by erasing ops after full krnl IR interpretation is done, and clean up & comment code. * Introduce permute, unroll operations. * More debug. * Remove std::set. * krnl.terminate fails to be converted. * Pass all tests, need to add legal ops as well as part of the conversion target. * Change test format to new permute spec. * Bug fix for nested iterate op lowering. * Simplify error reporting. * Fix compilation error. * Increase comments coverage. * Remove unnecessary imports. * Re-trigger Jenkins * Add permute/unroll tests. * Retrigger Jenkins * remove & (ref) for Attributes Co-authored-by: Tian Jin <tjingrant@gmail.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Syntax highlighting for mlir code in README (llvm#276) * Syntax highlighting for mlir code in README * Restart Jenkins Co-authored-by: Gheorghe-Teodor Bercea <gt.bercea@gmail.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com> Co-authored-by: Tian Jin <tjingrant@gmail.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * use print not dump Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add semicolon Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * syntax Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add code to preserve locations Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * format Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Emit the dynamic memory pool (llvm#290) * Reorganize main function. * Follow review comments. * Emit constants are globals in Krnl and LLVM dialects. * Add support for bundling dynamic memory pools. * Add dynamic bundling. * Clean-up code. * Clean-up file. * Add test for bundling dynamic memory pool. * Fixes. Simplify data structure. Add mixed test. * Remove unused import. Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Fix wrong type for llvm::loadop (llvm#293) Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Update llvm commit ID to 1d01fc1 (llvm#292) * Fix for LLVM revision D85495 * Fix for LLVM revision DD86121 * Fix for LLVM revision D85622 (f9dc2b7) TODO: Change preloadDialectsInContext to false Memo for previous fixes: D86121 (250f43d), D85495 (575b22b) * clang-format * Update llvm commit ID of README and clone-mlir.sh * Updated llvm commit ID of README.md * Fix for passing backend tests * Removed the commented code * Empty commit for triggering rebuild * Test multi-stage travis build * Specify stage order. * Empty commit for triggering rebuild * Update prereq.s390x.Dockerfile Make it possible to execute s390x prereq docker multiple times. * Build prereq for each arch * Fix multi-arch prereq build. * timeout at 40m * Update .travis.yml * add ppc64le prereq builder * Run ppc docker prereq build multiple times * Do not test branch update unless it's mater. * Fix dockerfile. * Fix typo in travis.yml. * Fix ppc64 docker file * Update .travis.yml * turn off metacopy on ppc64le * Update .travis.yml * Turn off metacopy. * Turn off metacopy inside Dockerfile in ppc64. * No sudo in Docker. * Remove metacopy config from Dockerfile. * Change base image to be bionic. * Using newer linux distro for ppc64. * Turn off metacopy in before_install. * Fix sudo permission issue. * Run docker info. * Allow amd64 docker file to be built multiple times * Support building amd64 prereq. * Fix amd64 docker file typo. * fix ppc64le dockerfile typo. * timeout from 40m -> 30m * 40m->30m * 40m->30m * fix bug preventing incremental build. * fix bug preventing incremental build. * Bump CircleCI cache version. * Push to production prereq container repository and condition prereq docker rebuild on commit message. * Rebuild prereq docker. * Move default script to top-level. * Python not properly installed. * amd64 -> x86 * Rebuild prereq docker. * Rebuild prereq docker. * Rebuild prereq docker. * Restart all CI. * Disallow cache on Jenkins docker build. * Restart zJenkins. * Restart zJenkins. Co-authored-by: Haruki Imai <imaihal@jp.ibm.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Using onnx-mlir through incremental stages (llvm#257) * Add lowering of Vector dialect for lower-all-llvm pass * Fix generating CallOp instructions when return type is void * Fix lowering of memref * Reformat using clang-format * Record more context. * Reflow comments. Co-authored-by: Tian Jin <tjingrant@gmail.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Dropout elimination & Conv Bugfix (llvm#297) * Dropout elimination. * Test VGG19. * Add shufflenet. * Fix grouped convolution bug. * Fix lit test failure. Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Rewrite shape and size OP (llvm#285) * add shape inference * Revert "add shape inference" This reverts commit f9d42f39e68e14b5648abccfc8617fff00244d16. * add rewrite rules * test cases * format * add constraint * response to review * response to review Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * initial code for handling custom ops (llvm#288) * initial code for handling custom ops * format Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * ShapeInference for SizeOp (llvm#299) * add shape inference * Revert "add shape inference" This reverts commit f9d42f39e68e14b5648abccfc8617fff00244d16. * shape inference * test case * format Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Gather ONNX to Kernel Lowering (llvm#294) * Define krnl.permute op. * Support krnl.permute operation. * Properly remove loop references. * Re-push, Github was down. * Need to debug interpretOp error. * Fix lowering bug by erasing ops after full krnl IR interpretation is done, and clean up & comment code. * Introduce permute, unroll operations. * More debug. * Remove std::set. * krnl.terminate fails to be converted. * Pass all tests, need to add legal ops as well as part of the conversion target. * Change test format to new permute spec. * Bug fix for nested iterate op lowering. * Simplify error reporting. * Fix compilation error. * Increase comments coverage. * Remove unnecessary imports. * Re-trigger Jenkins * Add permute/unroll tests. * Retrigger Jenkins * initial implementation of gather * added tests * format * remove affine load for second load, as it uses an indirection * changes suggested by reviewers * remove backend tests until I can verify them locally Co-authored-by: Tian Jin <tjingrant@gmail.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * fix option spelling Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * braces in wrong place Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * remove duplicate code from lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Simplify lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * remove attributes from lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add onnx-mlir-opt to tool names Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add printIR to second RUN Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * redo adding printIR Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * fix bug Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * format Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * fix typo in test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com> Co-authored-by: Tian Jin <tjingrant@gmail.com> Co-authored-by: Tung D. Le <tung@jp.ibm.com> Co-authored-by: Gheorghe-Teodor Bercea <gt.bercea@gmail.com> Co-authored-by: Haruki Imai <imaihal@jp.ibm.com> Co-authored-by: Kevin Wu <6334443+kwu91@users.noreply.github.com> Co-authored-by: chentong319 <chentong@us.ibm.com>
* Improve support for krnl.dim (llvm#317) * Reorganize main function. * Follow review comments. * Emit constants are globals in Krnl and LLVM dialects. * Make krnl dim more robust. * Format. * Update comments. * Change pass name. Signed-off-by: Tung D. Le <tung@jp.ibm.com> * Initial Location info support (llvm#302) * NFC: Attribute cleanup (remove references of attributes) (llvm#286) * Define krnl.permute op. * Support krnl.permute operation. * Properly remove loop references. * Re-push, Github was down. * Need to debug interpretOp error. * Fix lowering bug by erasing ops after full krnl IR interpretation is done, and clean up & comment code. * Introduce permute, unroll operations. * More debug. * Remove std::set. * krnl.terminate fails to be converted. * Pass all tests, need to add legal ops as well as part of the conversion target. * Change test format to new permute spec. * Bug fix for nested iterate op lowering. * Simplify error reporting. * Fix compilation error. * Increase comments coverage. * Remove unnecessary imports. * Re-trigger Jenkins * Add permute/unroll tests. * Retrigger Jenkins * remove & (ref) for Attributes Co-authored-by: Tian Jin <tjingrant@gmail.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Syntax highlighting for mlir code in README (llvm#276) * Syntax highlighting for mlir code in README * Restart Jenkins Co-authored-by: Gheorghe-Teodor Bercea <gt.bercea@gmail.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com> Co-authored-by: Tian Jin <tjingrant@gmail.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * use print not dump Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add semicolon Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * syntax Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add code to preserve locations Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * format Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Emit the dynamic memory pool (llvm#290) * Reorganize main function. * Follow review comments. * Emit constants are globals in Krnl and LLVM dialects. * Add support for bundling dynamic memory pools. * Add dynamic bundling. * Clean-up code. * Clean-up file. * Add test for bundling dynamic memory pool. * Fixes. Simplify data structure. Add mixed test. * Remove unused import. Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Fix wrong type for llvm::loadop (llvm#293) Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Update llvm commit ID to 1d01fc1 (llvm#292) * Fix for LLVM revision D85495 * Fix for LLVM revision DD86121 * Fix for LLVM revision D85622 (f9dc2b7) TODO: Change preloadDialectsInContext to false Memo for previous fixes: D86121 (250f43d), D85495 (575b22b) * clang-format * Update llvm commit ID of README and clone-mlir.sh * Updated llvm commit ID of README.md * Fix for passing backend tests * Removed the commented code * Empty commit for triggering rebuild * Test multi-stage travis build * Specify stage order. * Empty commit for triggering rebuild * Update prereq.s390x.Dockerfile Make it possible to execute s390x prereq docker multiple times. * Build prereq for each arch * Fix multi-arch prereq build. * timeout at 40m * Update .travis.yml * add ppc64le prereq builder * Run ppc docker prereq build multiple times * Do not test branch update unless it's mater. * Fix dockerfile. * Fix typo in travis.yml. * Fix ppc64 docker file * Update .travis.yml * turn off metacopy on ppc64le * Update .travis.yml * Turn off metacopy. * Turn off metacopy inside Dockerfile in ppc64. * No sudo in Docker. * Remove metacopy config from Dockerfile. * Change base image to be bionic. * Using newer linux distro for ppc64. * Turn off metacopy in before_install. * Fix sudo permission issue. * Run docker info. * Allow amd64 docker file to be built multiple times * Support building amd64 prereq. * Fix amd64 docker file typo. * fix ppc64le dockerfile typo. * timeout from 40m -> 30m * 40m->30m * 40m->30m * fix bug preventing incremental build. * fix bug preventing incremental build. * Bump CircleCI cache version. * Push to production prereq container repository and condition prereq docker rebuild on commit message. * Rebuild prereq docker. * Move default script to top-level. * Python not properly installed. * amd64 -> x86 * Rebuild prereq docker. * Rebuild prereq docker. * Rebuild prereq docker. * Restart all CI. * Disallow cache on Jenkins docker build. * Restart zJenkins. * Restart zJenkins. Co-authored-by: Haruki Imai <imaihal@jp.ibm.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Using onnx-mlir through incremental stages (llvm#257) * Add lowering of Vector dialect for lower-all-llvm pass * Fix generating CallOp instructions when return type is void * Fix lowering of memref * Reformat using clang-format * Record more context. * Reflow comments. Co-authored-by: Tian Jin <tjingrant@gmail.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Dropout elimination & Conv Bugfix (llvm#297) * Dropout elimination. * Test VGG19. * Add shufflenet. * Fix grouped convolution bug. * Fix lit test failure. Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Rewrite shape and size OP (llvm#285) * add shape inference * Revert "add shape inference" This reverts commit f9d42f39e68e14b5648abccfc8617fff00244d16. * add rewrite rules * test cases * format * add constraint * response to review * response to review Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * initial code for handling custom ops (llvm#288) * initial code for handling custom ops * format Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * ShapeInference for SizeOp (llvm#299) * add shape inference * Revert "add shape inference" This reverts commit f9d42f39e68e14b5648abccfc8617fff00244d16. * shape inference * test case * format Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Gather ONNX to Kernel Lowering (llvm#294) * Define krnl.permute op. * Support krnl.permute operation. * Properly remove loop references. * Re-push, Github was down. * Need to debug interpretOp error. * Fix lowering bug by erasing ops after full krnl IR interpretation is done, and clean up & comment code. * Introduce permute, unroll operations. * More debug. * Remove std::set. * krnl.terminate fails to be converted. * Pass all tests, need to add legal ops as well as part of the conversion target. * Change test format to new permute spec. * Bug fix for nested iterate op lowering. * Simplify error reporting. * Fix compilation error. * Increase comments coverage. * Remove unnecessary imports. * Re-trigger Jenkins * Add permute/unroll tests. * Retrigger Jenkins * initial implementation of gather * added tests * format * remove affine load for second load, as it uses an indirection * changes suggested by reviewers * remove backend tests until I can verify them locally Co-authored-by: Tian Jin <tjingrant@gmail.com> Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * fix option spelling Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * braces in wrong place Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * remove duplicate code from lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * Simplify lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * remove attributes from lit test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add onnx-mlir-opt to tool names Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * add printIR to second RUN Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * redo adding printIR Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * fix bug Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * format Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> * fix typo in test Signed-off-by: Kevin O'Brien <caomhin@us.ibm.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com> Co-authored-by: Tian Jin <tjingrant@gmail.com> Co-authored-by: Tung D. Le <tung@jp.ibm.com> Co-authored-by: Gheorghe-Teodor Bercea <gt.bercea@gmail.com> Co-authored-by: Haruki Imai <imaihal@jp.ibm.com> Co-authored-by: Kevin Wu <6334443+kwu91@users.noreply.github.com> Co-authored-by: chentong319 <chentong@us.ibm.com> Signed-off-by: Tung D. Le <tung@jp.ibm.com> * Support ReduceMean Signed-off-by: Tung D. Le <tung@jp.ibm.com> * Add lit tests Signed-off-by: Tung D. Le <tung@jp.ibm.com> * Fix unknown dimensions for type f32 Signed-off-by: Tung D. Le <tung@jp.ibm.com> Co-authored-by: Gheorghe-Teodor Bercea <gt.bercea@gmail.com> Co-authored-by: Kevin O'Brien <caomhin@us.ibm.com> Co-authored-by: Alexandre Eichenberger <alexe@us.ibm.com> Co-authored-by: Tian Jin <tjingrant@gmail.com> Co-authored-by: Haruki Imai <imaihal@jp.ibm.com> Co-authored-by: Kevin Wu <6334443+kwu91@users.noreply.github.com> Co-authored-by: chentong319 <chentong@us.ibm.com>
Add argmax lowering from torch to linalg.
Borrows heavily from the tosa lowering.
Has an issue at the moment, when I run something like:
npcomp-opt sample_argmax.mlir -torchscript-to-npcomp-backend-pipeline
I get:
sample_argmax.mlir:6:10: error: 'std.constant' op requires integer result types to be signless %1 = torch.aten.argmax %arg1, %int0, %true_0 : !torch.tensor, !torch.int, !torch.bool -> !torch.tensor ^ sample_argmax.mlir:6:10: note: see current operation: %6 = "std.constant"() {value = 0 : si64} : () -> si64
I struggled for awhile to realize that I had to make a case in refinetypes that changed the output dtype to int, but I'm not sure if I did that correctly. I'm probably missing something really obvious, but I'm not sure how to address this error.