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

Add EvalTranspose pattern to StablehloAggressiveFolder #2570

Merged
merged 3 commits into from
Oct 9, 2024

Conversation

mvpant
Copy link
Contributor

@mvpant mvpant commented Oct 1, 2024

This patch folds stablehlo.transpose operation with constant operand into stablehlo.constant.

I have considered doing this by iterating over source index space, i.e.

auto initialValue = *std::begin(data);
SmallVector<ElementType> result(resultType.getNumElements(), initialValue);

for (int64_t i = 0; i < operandType.getNumElements(); ++i) {
  auto srcDimIndex = delinearize(i, operandStrides);
  auto dstDimIndex = applyPermutation(srcDimIndex, permutation);
  auto dstLinearIndex = linearize(dstDimIndex, resultStrides);
  result[dstLinearIndex] = data[i];
}

but that requires preinitialization of result vector with some value, which is twice as slow on simple case:

func.func @eval_transpose() -> (tensor<5000x80x30xi32>) {
  %0 = stablehlo.iota dim = 0 : tensor<30x80x5000xi32>
  %1 = stablehlo.transpose %0, dims = [2, 1, 0] : (tensor<30x80x5000xi32>) -> tensor<5000x80x30xi32>
  func.return %1 : tensor<5000x80x30xi32>
}

@GleasonK GleasonK merged commit 15324c4 into openxla:main Oct 9, 2024
10 checks passed
@GleasonK
Copy link
Member

GleasonK commented Oct 9, 2024

@mvpant wanted to give a heads up that I'm going to be doing some re-organizing of StablehloAggressiveSimplification in the coming days (not planning to touch folders this week), and add some patterns from MHLO canonicalizers. If you don't have any active changes..maybe hold off on starting any for a few days to avoid a nasty merge conflict.

@mvpant
Copy link
Contributor Author

mvpant commented Oct 9, 2024

@mvpant wanted to give a heads up that I'm going to be doing some re-organizing of StablehloAggressiveSimplification in the coming days (not planning to touch folders this week), and add some patterns from MHLO canonicalizers. If you don't have any active changes..maybe hold off on starting any for a few days to avoid a nasty merge conflict.

@GleasonK sure. By the way, out of curiosity, what is the reason why stablehlo operations do not have <builtin> canonicalization using let hasCanonicalizer = 1; and folder let hasFolder = 1;? My guess is that this approach is intended to keep it stable for external users, ensuring that the generated output is predictable?

@GleasonK
Copy link
Member

My guess is that this approach is intended to keep it stable for external users, ensuring that the generated output is predictable?

Exactly this. I've had too many issues with folks not being able to opt-out of certain patterns being applied and then they invent "clever" solutions like barrier ops to prevent constant folding, or mirrors of convert ops to avoid folding ops that would indicate that model weights are quantized, etc. In the current design point I'm trying to make everything as opt-in as possible, and have a stable input format which serialized and deserializes without changing.

Also I had a followup question, I see a few uses of tensor::EmptyOp, and I'm thinking to replace these with stablehlo::ConstantOp : dense<>. Any objections to this? I figure if tensor ops are needed it can be an extension pattern which converts empty constants to tensor::empty that lives in a part of the codebase that supports tensors, as opposed to a builtin canonicalization pattern

@mvpant
Copy link
Contributor Author

mvpant commented Oct 29, 2024

... invent "clever" solutions like barrier ops to prevent constant folding, or mirrors of convert ops to avoid folding ops that would indicate that model weights are quantized, etc.

Eh, doesn't sound good, makes much more sense now to keep emitted operations as is.

Also I had a followup question, I see a few uses of tensor::EmptyOp, and I'm thinking to replace these with stablehlo::ConstantOp : dense<>. Any objections to this?

Nope. I think tensor::EmptyOp is handy at further stages because of the built-in bufferization stuff, but at stablehlo level it shouldn't make a difference. Having stablehlo.constant everywhere seems fine and more consistent.

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