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

Revert "[MLIR][TORCH] Only unroll prim loop-like ops within a torch.shape.calculate region" #3849

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

AmosLewis
Copy link
Collaborator

@AmosLewis AmosLewis commented Nov 1, 2024

Issue found when bump torch-mlir in iree iree-org/iree#18992. This revert can fix the issue iree-org/iree#18992 (comment) when bump.
This reverts commit 55ff110.
@Max191 has done this in last bump https://github.com/iree-org/torch-mlir/commits/integrates/llvm-20241024/ , but just in iree-org/torch-mlir, the revert patch has not been added in llvm/torch-mlir.

@AmosLewis AmosLewis changed the title Revert "[MLIR][TORCH] Only unroll prim loop-like ops within a `torch.… Revert "[MLIR][TORCH] Only unroll prim loop-like ops within a torch.shape.calculate region" Nov 1, 2024
@zjgarvey
Copy link
Collaborator

zjgarvey commented Nov 1, 2024

What is the issue?

Copy link
Collaborator

@jinchen62 jinchen62 left a comment

Choose a reason for hiding this comment

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

LGTM. Might need @zjgarvey to take a look.

@AmosLewis
Copy link
Collaborator Author

AmosLewis commented Nov 1, 2024

What is the issue?

@zjgarvey Slack has more info.

model.mlir:4:10: error: failed to legalize operation 'arith.sitofp' that was explicitly marked illegal
    %0 = torch.operator "onnx.TfIdfVectorizer"(%arg0) {torch.onnx.max_gram_length = 2 : si64, torch.onnx.max_skip_count = 0 : si64, torch.onnx.min_gram_length = 2 : si64, torch.onnx.mode = "TF", torch.onnx.ngram_counts = [0 : si64, 4 : si64], torch.onnx.ngram_indexes = [0 : si64, 1 : si64, 2 : si64, 3 : si64, 4 : si64, 5 : si64, 6 : si64], torch.onnx.pool_int64s = [2 : si64, 3 : si64, 5 : si64, 4 : si64, 5 : si64, 6 : si64, 7 : si64, 8 : si64, 6 : si64, 7 : si64]} : (!torch.vtensor<[2,6],si32>) -> !torch.vtensor<[2,7],f32> 
         ^
model.mlir:4:10: note: see current operation: %327 = "arith.sitofp"(<<UNKNOWN SSA VALUE>>) : (i64) -> f64
model.mlir:1:1: error: conversion to vm.module failed

@zjgarvey
Copy link
Collaborator

zjgarvey commented Nov 1, 2024

Yeah let me take a closer look. It doesn't make sense to revert this patch.

@zjgarvey
Copy link
Collaborator

zjgarvey commented Nov 1, 2024

I'm going to say we should not be reverting this.

If IREE is unable to manage something about scalar scf.for loops, then we should make a change in IREE to address this, rather than going back to indiscriminately unrolling IR loops during a pass that is supposed to perform shape inference in torch-mlir.

Alternatively, if something is wrong with TFIDF, we should be resolving that instead of reverting this patch.

Max posted an issue in IREE to track iree-org/iree#18961. I suggest we mark those three tests batched TFIDF tests as failures (regressions) in IREE. Then we can actually get someone to resolve it. Or not, since I've yet to see TFIDF show up in a single model.

@ScottTodd @Max191 Do you guys have any strong opinions on this?

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