The patch updates the C++ yaml code generation to support scalar operands as added in https://reviews.llvm.org/D104220.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
Mind rebasing this on head? Presubmits are failing due to not being on a commited CL.
Some nits and notes for the future. I like that this enables scalars and is much simpler than where the capture work was headed.
mlir/test/mlir-linalg-ods-gen/test-linalg-ods-yaml-gen.yaml | ||
---|---|---|
113–115 | Need to verify with the rest of this patch that this change in dims is intentional. | |
mlir/test/python/dialects/linalg/opsrun.py | ||
134 | Note for a follow-on patch: I believe this test file should be moved to the integration test directory. | |
mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-yaml-gen.cpp | ||
116 | Why switch this to an std::vector? (I know there are valid reasons to do so in this module, such as with type recursion, but I'm not seeing the rationale for this case) | |
152–153 | I believe you removed the temporary option above. Remove from this (now stale) comment? |
mlir/test/mlir-linalg-ods-gen/test-linalg-ods-yaml-gen.yaml | ||
---|---|---|
113–115 | I swapped the shape since the input I is read transposed (the indexing map is also swapped). It has no direct effect here since we only test the yaml to c++ translation. | |
mlir/test/python/dialects/linalg/opsrun.py | ||
134 | @stellaraccident Would you move it under a new integration test directory in the python subfolder (e.g. mlir/python/test/integration) or in the existing integration test directory (e.g. mlir/test/integration). I think having all python code in the same subfolder (mlir/python/) seems preferable? | |
mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-yaml-gen.cpp | ||
116 | When using SmallVector<ScalarAssign> I ran into a problem with a static assert checking the size (on Windows only). Setting a fixed size resolved the problem and may overall be more consistent. I just thought std::vector is more on the safe side since struct is big (close to 256 Byte). But assuming the parent struct is most likely heap allocated I think using inline storage is fine. |
mlir/tools/mlir-linalg-ods-gen/mlir-linalg-ods-yaml-gen.cpp | ||
---|---|---|
116 | Thanks and sg. It is fine to leave it as vector given this. Was just wondering the rationale for the change. |
Need to verify with the rest of this patch that this change in dims is intentional.