Skip to content
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

Sever IREE repo connection with mlir-hlo #12678

Closed
julianwa opened this issue Mar 18, 2023 · 9 comments · Fixed by #14094
Closed

Sever IREE repo connection with mlir-hlo #12678

julianwa opened this issue Mar 18, 2023 · 9 comments · Fixed by #14094
Assignees
Labels
infrastructure Relating to build systems, CI, or testing integrations/stablehlo StableHLO (JAX/TensorFlow/etc) import and conversion

Comments

@julianwa
Copy link
Member

julianwa commented Mar 18, 2023

Issues with IREE's mlir-hlo dependency have been blocking integrates for 2 weeks. We've shared the intention to cut ties with this dependency, and now we need to do so expediently to unblock the integrates.

Very high-level description of what's needed:

  • Create proposal, making sure to confirm with @GMNGeoffrey and @jpienaar that the proposal is coherent when integrating IREE back into Google.
  • Extract some files out of mlir-hlo, organize them appropriately in IREE, delete submodule.
  • Submit when tests pass.

More detailed plan should be discussed with Magnus Hyttsten (not sure his github handle).

@julianwa julianwa added the infrastructure Relating to build systems, CI, or testing label Mar 18, 2023
@aaron-schneider aaron-schneider removed their assignment Mar 22, 2023
kuhar added a commit that referenced this issue Mar 23, 2023
Also update the bazel-cmake target mapping to avoid pulling in unused
cmake targets.

Issue: #12678
@jpienaar jpienaar added the integrations/stablehlo StableHLO (JAX/TensorFlow/etc) import and conversion label Mar 27, 2023
@aaron-schneider
Copy link

Hi - double checking on this P0 issue. More to say? Ok to close or lower priority? Thanks!

@GMNGeoffrey
Copy link
Contributor

@kuhar is actively working on this, but I would say it's not P0 and requires some consideration.

kuhar added a commit that referenced this issue Apr 11, 2023
This is a port of the MHLO to Linalg lowering from
https://github.com/tensorflow/mlir-hlo.
The iree-mhlo-fork commit used to port the conversion:
1f096a793ab7f73ae8f62deb8b6502c543763ca1.
The imported files are relicensed under the [Google
CLA](https://cla.developers.google.com/about/google-individual) from the
Apache 2.0 license (Tensorflow) to the nearly-identical Apache 2.0 with
the LLVM exceptions license (IREE).

The initial import covers the lowering of StableHLO ops that can be
trivially mapped to their MHLO counterparts. More complicated ops, like
convolutions, gather, or rng, are not ported yet.

In porting MHLO conversions and tests to operate on StableHLO ops, I
changed all namespaces, header guards, and copyright headers, and
formatted all files to match the conventions used by IREE.

Any addition modifications were a non-goal. I plan to reorganizanize and
clean this up further after the initial porting.

Issue: #12678
kuhar added a commit that referenced this issue Apr 11, 2023
This is primarily to improve compilation times when iterating on the
rest of StableHLO to Linalg conversion patterns. These pointwise
patterns create a large number of class template instantiations and take
>20s to compile on my machine.

Also clean up the code:
-  Use free cast functions
-  Make the conversion patterns final
-  Prefer static/free helper functions

Issue: #12678
kuhar added a commit to kuhar/iree that referenced this issue Apr 11, 2023
This is primarily to reduce build times and length of the main stablehlo
to linalg conversion pass file.

Also clean up the code:
-  Use `LogicalResult` for verification success/failure
-  Use free cast functions.
-  Replace some local `auto` with the actual type where not obvious.

Issue: iree-org#12678
kuhar added a commit that referenced this issue Apr 11, 2023
This is primarily to reduce build times and length of the main stablehlo
to linalg conversion pass file.

Also clean up the code:
-  Use `LogicalResult` for verification success/failure
-  Use free cast functions
-  Make patterns `final`
-  Replace some local `auto` with the actual type where not obvious

Issue: #12678
kuhar added a commit to kuhar/iree that referenced this issue Apr 12, 2023
This moves the pointwise StableHLO op to `linalg.generic` conversion out
of the common header and to the matching source file.

In addition, unify the common precondition checks code used by both
pointwise patterns and pull it out to a non-template function. This is
to reduce the amount of duplicated code. This reduces the compilation
time of this source file from 28s to 22s on my machine.

Also clean up the moved pattern.

Issue: iree-org#12678
kuhar added a commit that referenced this issue Apr 12, 2023
…13044)

This moves the pointwise StableHLO op to `linalg.generic` conversion out
of the common header and to the matching source file.

In addition, unify the common precondition checks code used by both
pointwise patterns and pull it out to a non-template function. This is
to reduce the amount of duplicated code. This reduces the compilation
time of this source file from 28s to 22s on my machine.

Also clean up the moved pattern.

Issue: #12678
kuhar added a commit to kuhar/iree that referenced this issue Apr 12, 2023
I went over the list of all pointwise ops that we currently handle.
These fell through the cracks during the initial import (iree-org#12957).

Two ops are not tested: `ComplexOp` and `NotOp`, but these do not have
any tests in mlir-hlo either.

Next, I plan to split the linalg lowering tests into a few smaller
test files.

Issue: iree-org#12678
kuhar added a commit that referenced this issue Apr 12, 2023
I went over the list of all pointwise ops that we currently handle.
These fell through the cracks during the initial import (#12957).

Two ops are not tested: `ComplexOp` and `NotOp`, but these do not have
any tests in mlir-hlo either.

Next, I plan to split the linalg lowering tests into a few smaller test
files.

Issue: #12678
kuhar added a commit to kuhar/iree that referenced this issue Apr 12, 2023
Factor out pointwise and dot prod op tests from the main test file.
This matches the source file organization of the conversion patterns.

Do not run with the 'enable-primitive-ops' option on tests that are not
affected by it.

Issue: iree-org#12678
kuhar added a commit to kuhar/iree that referenced this issue Apr 12, 2023
Factor out pointwise and dot prod op tests from the main test file.
This matches the source file organization of the conversion patterns.

Do not run with the 'enable-primitive-ops' option on tests that are not
affected by it.

Issue: iree-org#12678
kuhar added a commit that referenced this issue Apr 12, 2023
Factor out pointwise and dot prod op tests from the main test file. This
matches the source file organization of the conversion patterns.

The test cases are not modified. The only difference is that now we do
not run with the 'enable-primitive-ops' option on tests files that are
not affected by it.

Issue: #12678
kuhar added a commit to kuhar/iree that referenced this issue Apr 12, 2023
…ests

This option invalidates all variable bindings after `CHECK-LABEL`.

Enabling this option revealed a couple of issues in dot prod tests,
where incorrect variables from previous functions were used.

Issue: iree-org#12678
kuhar added a commit that referenced this issue Apr 13, 2023
Add tests for `stablehlo.not` and `stablehlo.complex`. These were not
present in the equivalent mlir-hlo tests.

Issue: #12678
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
…-org#13755)

These should cover the same rewrites as the equivalent
folds/canonicalization functions from MHLO, but the implementation is
not the same.

Issue: iree-org#12678
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
This pattern folds gather into slice (+ reshape) and is ported from
MHLO.

Issue: iree-org#12678
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
This pattern fell through the cracks during the initial porting of
hlo-to-linalg lowering in iree-org#12957.

With this pattern and the most recent canon patterns, we produce the
same code as the mhlo input conversion pipeline on the input from
iree-org#13729.

Also fixed issues with undefined FileCheck variables in tests.

Issue: iree-org#12678
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
These are based on the equivalent IREE tests for MHLO.

Issue: iree-org#12678
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
These require some stablehlo canonicalization patterns during lowering
to IREE dialects.

I also discovered some issues with the generic syntax
(openxla/stablehlo#1539) and updated the tests
to use the custom assembly format.

Issue: iree-org#12678
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
The list of excludes is the same as for the MHLO equivalent. I also
checked and the generated code is identical.

Issue: iree-org#12678
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
This test is disabled on rv32 due to known issues with the musl lib. I
checked the generated code and it looks identical to the mhlo output
now.

Issue: iree-org#12678
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
As we continue the migration to the stablehlo input conversion pipeline,
mark the mhlo pipeline as deprecated and rename the option to
`--iree-input-type=mhlo_legacy`.

This was proposed and announced in the following RFC:
https://groups.google.com/g/iree-discuss/c/s6dBpDtWhtk.

Users are encouraged to try the new pipeline:
`--iree-input-type=stablehlo` and report any issues or missing features,
as we plan to drop the legacy mhlo pipeline in a few weeks.

Update tests to use the new flag name. Update documents to suggest
either the new pipeline or the new flag.

Next, we will have to migrate existing test and samples to use StableHLO
as the input format:
iree-org#13869.

Issue: iree-org#12678
NatashaKnk pushed a commit to NatashaKnk/iree that referenced this issue Jul 6, 2023
Add a missing test for `chlo.broadcast_add`.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
These are based on the equivalent fold and canonicalizer from MHLO.

Also run stablehlo canonicalization pass between other lowering passes. 

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
…-org#13755)

These should cover the same rewrites as the equivalent
folds/canonicalization functions from MHLO, but the implementation is
not the same.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
This pattern folds gather into slice (+ reshape) and is ported from
MHLO.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
This pattern fell through the cracks during the initial porting of
hlo-to-linalg lowering in iree-org#12957.

With this pattern and the most recent canon patterns, we produce the
same code as the mhlo input conversion pipeline on the input from
iree-org#13729.

Also fixed issues with undefined FileCheck variables in tests.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
These are based on the equivalent IREE tests for MHLO.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
These require some stablehlo canonicalization patterns during lowering
to IREE dialects.

I also discovered some issues with the generic syntax
(openxla/stablehlo#1539) and updated the tests
to use the custom assembly format.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
The list of excludes is the same as for the MHLO equivalent. I also
checked and the generated code is identical.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
This test is disabled on rv32 due to known issues with the musl lib. I
checked the generated code and it looks identical to the mhlo output
now.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
As we continue the migration to the stablehlo input conversion pipeline,
mark the mhlo pipeline as deprecated and rename the option to
`--iree-input-type=mhlo_legacy`.

This was proposed and announced in the following RFC:
https://groups.google.com/g/iree-discuss/c/s6dBpDtWhtk.

Users are encouraged to try the new pipeline:
`--iree-input-type=stablehlo` and report any issues or missing features,
as we plan to drop the legacy mhlo pipeline in a few weeks.

Update tests to use the new flag name. Update documents to suggest
either the new pipeline or the new flag.

Next, we will have to migrate existing test and samples to use StableHLO
as the input format:
iree-org#13869.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
Add a missing test for `chlo.broadcast_add`.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
Turns out that linalg lowering depends on these.

The code is ported from equivalent folds and rewrite patterns in the
mlir-hlo repo. The only notable difference is that this implementation
fixes bugs with rewrite patterns performing op updates without going
through the pattern rewriter. Also added a new test for the second noop
case.

Fixes: iree-org#14042
Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
Check if reduce ops are supported. This is so that these patterns can be
given any reduce, even those that would be normally folded away by canon
patterns.

Issue: iree-org#14042
Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
Drop the MHLO input conversion pipeline, which has been deprecated for
over a week. The StableHLO pipeline is the direct replacement. See the
announcement thread for more context:
https://groups.google.com/g/iree-discuss/c/s6dBpDtWhtk.

This still uses the copy of stablehlo from the mlir-hlo repo -- we will
switch to the stablehlo repo in a follow-up PR.

Issue: iree-org#12678
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
nhasabni pushed a commit to plaidml/iree that referenced this issue Aug 24, 2023
Use the stablehlo submodule to provide stablehlo, since we do not need
the 'mhlo' part or mlir-hlo anymore.

Fixes: iree-org#12678

---------

Co-authored-by: Scott Todd <scotttodd@google.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
infrastructure Relating to build systems, CI, or testing integrations/stablehlo StableHLO (JAX/TensorFlow/etc) import and conversion
Projects
None yet
Development

Successfully merging a pull request may close this issue.

6 participants