Added support for i4 Const-eval for Tensors #16321

bviyer · 2024-02-05T20:17:24Z

This patch mostly included work of Max Dawkins in
this draft #15682

Co-authored-by: Max191 maximilian@nod-labs.com

Max191

I think there is still some more needed support (or bitcasting tricks) for subbyte non-tensor types before we can flip the switch in const-eval.

Max191 · 2024-02-07T14:39:19Z

compiler/src/iree/compiler/Dialect/Util/Analysis/Constant/OpOracle.cpp

  if (auto integerType = llvm::dyn_cast<IntegerType>(
          getElementTypeOrSelf(info->constValue.getType()))) {
-    if (integerType.getWidth() % 8 != 0) {
+    // Allow i4 hoisting.


AFAIK, we still don't have support through the runtime for evaluating subbyte single elements (see the comments following #15682 (comment)). Has this changed since that discussion? If not, then we need to support that (or maybe do some bitcasting) before flipping this switch.

compiler/src/iree/compiler/Dialect/Util/Analysis/Constant/OpOracle.cpp

compiler/src/iree/compiler/GlobalOptimization/test/flow_hoist_into_globals.mlir

Max191

Thanks! This looks good to me now. I added my email to the PR so the cla will stop failing.

EDIT: Hmm, that seems to have not fixed it. I'm not sure why it is failing then

bviyer · 2024-02-17T04:56:17Z

I am not sure how to fix this conflict. I tried all these steps mentionedin this list and still having this issue.

Checkout via command line
If the conflicts on this branch are too complex to resolve in the web editor, you can check it out via command line to resolve the conflicts.

https://github.com/bviyer/iree.git
Step 1: From your project repository, check out a new branch and test the changes.

git checkout -b bviyer-balaji/322157900 main
git pull https://github.com/bviyer/iree.git balaji/322157900
Step 2: Merge the changes and update on GitHub.

git checkout main
git merge --no-ff bviyer-balaji/322157900
git push origin main

@Max191 or @dcaballe Can you please tell me what I could be doing wrong?

ScottTodd · 2024-02-17T05:44:33Z

Thanks! This looks good to me now. I added my email to the PR so the cla will stop failing.

EDIT: Hmm, that seems to have not fixed it. I'm not sure why it is failing then

The CLA check is looking at the commits, not the PR description. The co-authored fields on each commit need some specific syntax.

I am not sure how to fix this conflict.

You should at least revert the changes to third_party/llvm-project. It might be easier to resolve with an interactive rebase and squashed commits instead of a merge.

dcaballe · 2024-02-20T18:36:05Z

compiler/src/iree/compiler/ConstEval/JitGlobals.cpp

+    // the `eval_i4_tensor` test in `jit_globals.mlir` to fail.
+    // TODO: Remove this if-statement and the one wrapping around
+    //       addElementType for i4 when the support is enabled.
+    if (requestedTargetBackend != "vmvx" && hasRequestedTargetBackend)


Since backends require specific support, should we enable this only for llvmcpu instead? I'm not sure if other backend beyond vmvx have the proper support.

+1 to this. I think it would probably work on the GPU backends, but I'm also not sure. Let's only enable for llvmcpu and leave it as a TODO to test and enable i4 on other backends. Let's open up an issue to track this as well and you can leave a TODO comment like:
// TODO(#12345): Enable on other backends once this has been tested outside llvm-cpu.

Max191

Let's just enable for llvm-cpu for now, and then we can follow up by adding other backends.

Max191 · 2024-02-20T19:49:43Z

compiler/src/iree/compiler/ConstEval/JitGlobals.cpp

+    // the `eval_i4_tensor` test in `jit_globals.mlir` to fail.
+    // TODO: Remove this if-statement and the one wrapping around
+    //       addElementType for i4 when the support is enabled.
+    if (requestedTargetBackend != "vmvx" && hasRequestedTargetBackend)


+1 to this. I think it would probably work on the GPU backends, but I'm also not sure. Let's only enable for llvmcpu and leave it as a TODO to test and enable i4 on other backends. Let's open up an issue to track this as well and you can leave a TODO comment like:
// TODO(#12345): Enable on other backends once this has been tested outside llvm-cpu.

This patch mostly included work of Max Dawkins in this draft iree-org#15682 Co-authored-by: Max191 <maximilian@nod-labs.com>

ScottTodd · 2024-04-17T20:56:13Z

compiler/src/iree/compiler/ConstEval/JitGlobals.cpp

+    if (requestedTargetBackend == "llvm-cpu" && hasRequestedTargetBackend)
+      s.addElementType(b.getIntegerType(4));


I'm finding on #17075 that this is broken (crashes, ASan error reports, etc. when trying to run basic unit tests). There were also no relevant test cases changed in jit_globals.mlir. Might revert this full PR.

See also the discussion on Discord here

when I flip the test to use llvm-cpu, I get buffer.c:449: OUT_OF_RANGE; attempted to access an address outside of the valid buffer range (offset=0, adjusted_length=30, end=29, buffer byte_length=15); on the eval_i4_tensor test case from code around here: https://github.com/openxla/iree/blob/fdfe344a8b7f3ab0bad14b6cc543f301da0d0acd/compiler/src/iree/compiler/ConstEval/Runtime.cpp#L399-L409

and ASan logs here: https://github.com/openxla/iree/actions/runs/8727055167/job/23943706482?pr=17075#step:4:12961

* Fixed #17070 by updating the CMake options needed for ConstEval * Replaced `IREE_CHECK_OK` usage with error handling * Refactored `test/jit_globals.mlir`, adding coverage for llvm-cpu (since that is actually running by default) * I tried to keep all test cases in one file, but `--verify-diagnostics` isn't compatible with that style of lit testing AFAICT * This uncovered some bugs in #16321 / missing support for i4 types

…rg#17075) * Fixed iree-org#17070 by updating the CMake options needed for ConstEval * Replaced `IREE_CHECK_OK` usage with error handling * Refactored `test/jit_globals.mlir`, adding coverage for llvm-cpu (since that is actually running by default) * I tried to keep all test cases in one file, but `--verify-diagnostics` isn't compatible with that style of lit testing AFAICT * This uncovered some bugs in iree-org#16321 / missing support for i4 types

…rg#17075) * Fixed iree-org#17070 by updating the CMake options needed for ConstEval * Replaced `IREE_CHECK_OK` usage with error handling * Refactored `test/jit_globals.mlir`, adding coverage for llvm-cpu (since that is actually running by default) * I tried to keep all test cases in one file, but `--verify-diagnostics` isn't compatible with that style of lit testing AFAICT * This uncovered some bugs in iree-org#16321 / missing support for i4 types Signed-off-by: Lubo Litchev <lubol@google.com>

bviyer requested review from Max191 and dcaballe February 5, 2024 20:17

bviyer requested review from benvanik, hanhanW and stellaraccident as code owners February 5, 2024 20:17

bviyer mentioned this pull request Feb 5, 2024

Support i4 types in ConstEval #15682

Closed

Max191 requested changes Feb 7, 2024

View reviewed changes

bviyer force-pushed the balaji/322157900 branch 2 times, most recently from 80bc76a to 6e50365 Compare February 16, 2024 21:30

bviyer requested a review from Max191 February 16, 2024 21:31

Max191 approved these changes Feb 16, 2024

View reviewed changes

dcaballe approved these changes Feb 16, 2024

View reviewed changes

bviyer force-pushed the balaji/322157900 branch from d052033 to 88ed355 Compare February 20, 2024 16:28

bviyer requested review from Max191 and dcaballe February 20, 2024 18:29

dcaballe reviewed Feb 20, 2024

View reviewed changes

Max191 approved these changes Feb 20, 2024

View reviewed changes

Added support for i4 Const Eval for Tensors

ef55557

This patch mostly included work of Max Dawkins in this draft iree-org#15682 Co-authored-by: Max191 <maximilian@nod-labs.com>

bviyer force-pushed the balaji/322157900 branch from f99341e to ef55557 Compare February 20, 2024 20:21

bviyer changed the title ~~Added support for i4 Const-eval for Scalars~~ Added support for i4 Const-eval for Tensors Feb 20, 2024

bviyer merged commit 218a5e6 into iree-org:main Feb 20, 2024

ScottTodd mentioned this pull request Apr 17, 2024

Harden how ConstEval uses llvm-cpu and the runtime libraries. #17075

Merged

ScottTodd reviewed Apr 17, 2024

View reviewed changes

		if (requestedTargetBackend == "llvm-cpu" && hasRequestedTargetBackend)
		s.addElementType(b.getIntegerType(4));

Added support for i4 Const-eval for Tensors #16321

Added support for i4 Const-eval for Tensors #16321

Uh oh!

Conversation

bviyer commented Feb 5, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Max191 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Max191 left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bviyer commented Feb 17, 2024

Uh oh!

ScottTodd commented Feb 17, 2024

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Max191 left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

bviyer commented Feb 5, 2024 •

edited

Loading

Max191 left a comment •

edited

Loading