Skip to content

Conversation

@GlebKazantaev
Copy link
Collaborator

Description

In this PR I enable VerifyBackendContract pass as a part of LTC backend contract to check that MLIR satisfies backend requirements. See #1460 for more details.

Changes

  • Update TorchMlirLoweringContext::Build with VerifyBackendContract pass execution.
  • Updated TorchMlirComputation to accept MlirModule.
  • Removed useless VerifyBackendContract pass parametrization.

This PR depends on changes in PyTorch pytorch/pytorch#92095 so once they land I will mark this PR as ready for review.

@glebk-cerebras glebk-cerebras force-pushed the gleb/verify_backend_contract branch from ade8674 to d7ce401 Compare January 19, 2023 22:13
@GlebKazantaev GlebKazantaev marked this pull request as ready for review January 19, 2023 22:32
@GlebKazantaev
Copy link
Collaborator Author

GlebKazantaev commented Jan 19, 2023

@ramiro050 I just realized that you've added parameterization for VerifyBackendContract pass here: #1705
Just curios, in which case we need to check that IR was decomposed? In my understanding decompose is an option of LowerToBackendContract pass which take care about all the checks after lowering and VerifyBackendContract pass should check the types consistency, shape existence, value semantics (at least by default).

I also found that in generated CAPI there is no way to specify arguments. All arguments are just embedded to the cpp file. For example in ./torch-mlir/build/cmake_build/tools/torch-mlir/include/torch-mlir/Dialect/Torch/Transforms/Transforms.capi.cpp.inc :

MlirPass mlirCreateLowerToBackendContract(void) {
  return wrap(
    mlir::torch::Torch::createLowerToBackendContractPass(
      /*maxIterations=*/10, /*decompose=*/true, /*backendLegalOps=*/{})
  .release());
}

all parameters were set, and mlirCreateLowerToBackendContract doesn't take any arguments. The same thing I see for all other MLIR passes. So probably it smth not yet supported... Do you have any ideas?

As a workaround we can have two separate passes for VerifyBackendContract for decompose=false and VerifyBackendContractDecomposed for decompose=true.

@silvasean
Copy link
Contributor

LGTM pending Ramiro's response on the options.

@ramiro050
Copy link
Collaborator

ramiro050 commented Jan 20, 2023

@ramiro050 I just realized that you've added parameterization for VerifyBackendContract pass here: #1705
Just curios, in which case we need to check that IR was decomposed? In my understanding decompose is an option of LowerToBackendContract pass which take care about all the checks after lowering and VerifyBackendContract pass should check the types consistency, shape existence, value semantics (at least by default).

The parameters were added to be consistent with the definition of the backend contract, which includes decompositions. However, the verifyBackendContract pass is only used by LTC, so I think we should just rename the pass to verifyBackendContractAssumingNoDecompositions to make it very clear to someone reading the code what is being verified.

@glebk-cerebras glebk-cerebras force-pushed the gleb/verify_backend_contract branch from 5ac95d1 to 8248863 Compare January 23, 2023 18:54
@GlebKazantaev
Copy link
Collaborator Author

GlebKazantaev commented Jan 23, 2023

@ramiro050 I renamed the pass. Could you please take a look? If everything is okay we can get it merged.

Copy link
Collaborator

@ramiro050 ramiro050 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Just a small nit

@GlebKazantaev GlebKazantaev merged commit 3930588 into llvm:main Jan 25, 2023
@silvasean
Copy link
Contributor

Thanks for pushing this through @GlebKazantaev !!

gpetters94 pushed a commit to gpetters94/mlir-npcomp that referenced this pull request May 10, 2023
* Enable VerifyBackendContract in LTC backend

* Update VerifyBackendContract pass

* Move convert_scalar_implicit to jit_utils

* Rename VerifyBackendContract to VerifyBackendContractNoDecompositions

* Update verify-backend-contract-error.mlir test
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants