Skip to content

Conversation

panickal-xmos
Copy link
Contributor

There are two changes :

  • Adds a new td_library target to be able to include lce_ops.td externally. In current MLIR and Tensorflow upstream, all td files are defined as part of td_library targets. This will probably need to be done for all td files when compute-engine is upgraded to Tensorflow 2.7
  • Adds a missing dependency to @llvm-project//mlir:QuantOps. Missing header dependencies are enforced when building with later bazel versions.

@panickal-xmos
Copy link
Contributor Author

@lgeiger The MLIR test fail seems to be due to credentials. Can I do something to fix this?

@lgeiger
Copy link
Member

lgeiger commented Dec 14, 2021

The MLIR test fail seems to be due to credentials. Can I do something to fix this?

Sorry about that, it looks like there was a bug in the latest gcloud action release that broke our CI for PRs from forks. This is now fixed on main. @panickal-xmos can you rebase your PR onto main to fix the CI failure here?

Copy link
Member

@lgeiger lgeiger left a comment

Choose a reason for hiding this comment

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

Thank you very much for the PR.

When looking at the upstream build file is seems like build targets depend on the td_library instead of the *.td files directly. Could you change the dependency definitions in this file from ir/lce_ops.td to the new lce_ops_tf_file to make it consistent and to make sure that this build target is also tested properly on CI.

@lgeiger lgeiger added the internal-improvement Internal Improvements and Maintenance label Dec 14, 2021
Copy link
Member

@lgeiger lgeiger left a comment

Choose a reason for hiding this comment

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

Looks great! Thanks for the PR 🎉

@lgeiger lgeiger merged commit 494eb5d into larq:main Dec 15, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
internal-improvement Internal Improvements and Maintenance
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants