Add NumToTensor #393

gpetters94 · 2021-11-02T07:41:16Z

This looks to be working, but this is the first time I've had to write a test case in mlir.

pashu123

LGTM, Some NFC.

test/Conversion/TorchToLinalg/basic.mlir

lib/Dialect/Torch/Transforms/RefineTypes.cpp

pashu123 · 2021-11-02T15:03:33Z

lib/Dialect/Torch/Transforms/RefineTypes.cpp

@@ -412,6 +412,8 @@ class TypeAnalyzer : public ForwardDataFlowAnalysis<ValueKnowledge> {
      return visitAtenMatmulOp(matmul, operands);
    } else if (auto softmaxIntOp = dyn_cast<AtenSoftmaxIntOp>(op)) {
      return visitAtenSoftmaxIntOp(softmaxIntOp, operands);
+    } else if (auto nts = dyn_cast<PrimNumToTensorScalarOp>(op)) {


NIT: I think we should follow the naming convention, i.e., name it as numToTensorOp rather than nts.

This comment is not addressed yet.

lib/Dialect/Torch/Transforms/RefineTypes.cpp

lib/Conversion/TorchToLinalg/TorchToLinalg.cpp

lib/Dialect/Torch/Transforms/RefineTypes.cpp

ramiro050 · 2021-11-02T16:41:36Z

I've created a PR #395 with a simple script you can use to test your implementation e2e. Just replace the graph_str with something like

graph_str = """\
graph(%p0 : Tensor, %p1 : Tensor):
  %0 : float = prim::Constant[value=0.123]()
  %1 : Tensor = prim::NumToTensor(%0)
  return (%1)
"""

You can also test different input types by changing the prim::Constant. For example, to test ints do

%0 : int = prim::Constant[value=3]()

silvasean · 2021-11-02T16:47:22Z

You should be able to look at the source code location in the IR to find the line in the Python code that results in this op being created and create a Python e2e test.

ramiro050 · 2021-11-02T16:50:12Z

@silvasean, unfortunately there is no source location on this graph being used for the Bert ops

https://gist.github.com/cathyzhyi/8b9e3ec28f66df2c119795b13abcc1f8

silvasean · 2021-11-02T16:59:36Z

Hmmm... ok. In that case, it is more difficult, but I think the effort to discover a Python test case is valuable. I just searched for "NumToTensor" in PyTorch and found that something like return _to_tensor(1) creates it.

silvasean · 2021-11-02T17:00:05Z

that might need to be guarded with torch.jit.is_scripting()

silvasean · 2021-11-02T17:03:47Z

Ha, I knew this sounded familiar, we actually have it in one of test cases already :)

torch-mlir/test/python/importer/jit_ir/node_import/prim.py

Line 17 in 127c7d8

# CHECK-LABEL: func @__torch__.prim_NumToTensor(

ramiro050 · 2021-11-02T17:04:19Z

Hmmm... ok. In that case, it is more difficult, but I think the effort to discover a Python test case is valuable.

I agree I just thought it would make debugging much easier if people with ops that are difficult to get information on have a simple way to quickly test their implementations locally

silvasean · 2021-11-02T17:11:02Z

Hopefully we don't have too many ops like this (they are pretty special). Let's see if it becomes a problem.

gpetters94 · 2021-11-03T00:13:20Z

Okay, I've addressed the issues. Should I be writing a new test case, or does the one that Sean pointed out suffice?

cathyzhyi · 2021-11-03T00:37:23Z

lib/Dialect/Torch/Transforms/RefineTypes.cpp

+  auto knowledge =
+      ValueKnowledge::getNotNonePessimisticValueState(op->getContext());
+  knowledge.hasSizes = true;
+  knowledge.dtype = getDefaultDtypeForTorchScalar(op->getResultTypes()[0]);


should use the scalar type op.a().getType() here.

Please refer to https://github.com/llvm/torch-mlir/blob/main/test/Dialect/Torch/refine-types.mlir to add unit test for refine type.

cathyzhyi · 2021-11-03T00:42:48Z

lib/Dialect/Torch/Transforms/RefineTypes.cpp

@@ -412,6 +412,8 @@ class TypeAnalyzer : public ForwardDataFlowAnalysis<ValueKnowledge> {
      return visitAtenMatmulOp(matmul, operands);
    } else if (auto softmaxIntOp = dyn_cast<AtenSoftmaxIntOp>(op)) {
      return visitAtenSoftmaxIntOp(softmaxIntOp, operands);
+    } else if (auto nts = dyn_cast<PrimNumToTensorScalarOp>(op)) {


This comment is not addressed yet.

ramiro050 · 2021-11-03T00:43:29Z

I think you should still add an e2e test. From what I can tell, the test Sean pointed out does not perform any passes on the MLIR, so your code for RefineTypes.cpp is not being tested by that test.

cathyzhyi · 2021-11-04T14:42:27Z

e2e_testing/torchscript/basic.py

+
+@register_test_case(module_factory=lambda: NumToTensorModule())
+def NumToTensorModule_basic(module, tu: TestUtils):
+    prim_NumToTensor(1)


nit: can you add a newline here? github is showing a red sign because no newline at the end of the file.

cathyzhyi · 2021-11-04T14:44:27Z

test/Dialect/Torch/refine-types.mlir

+func @aten_prim_numtotensor(%arg0: !torch.int) -> !torch.tensor {
+  %0 = torch.prim.NumToTensor.Scalar %arg0: !torch.int -> !torch.tensor
+  return %0: !torch.tensor
+}


nit: also need a new line here.

@gpetters94 reminder: this is not addressed yet :)

ramiro050 · 2021-11-04T15:56:07Z

e2e_testing/torchscript/basic.py

+        return _to_tensor(1)
+
+@torch.jit.script
+def prim_NumToTensor(i: int):


What's the reason for having this extra function?

I wasn't able to properly guard the forward function, so this is a hackey way to get it to run.

ramiro050 · 2021-11-04T16:58:28Z

I just realized something that applies here and might be useful for the future. This is based on the comment Chilli posted on the discord (https://discord.com/channels/636084430946959380/742573221882364009/904914843683942431)

I was looking into how this is implemented, and torch.ops is quite magical. See this docstring:

https://github.com/pytorch/pytorch/blob/master/torch/_ops.py#L32-L51

It works for any namespace! This means that any op that we cannot find, we can just use the JITOperator name to access it. In this case, you can do

torch.ops.prim.NumToTensor(some_number)

It also extracts some info about the op that can be accessed with help:

> help(torch.ops.prim.NumToTensor)

Help on built-in function NumToTensor:

NumToTensor(...) method of builtins.PyCapsule instance
    NumToTensor(*args, **kwargs) -> object

    Automatically bound operator 'prim::NumToTensor' with schema(s):
      prim::NumToTensor.Scalar(Scalar a) -> (Tensor)
      prim::NumToTensor.bool(bool a) -> (Tensor)

Edit: Of course, _to_tensor works perfectly fine here :)

ramiro050 · 2021-11-04T17:46:26Z

Actually, if you do use torch.ops.prim.NumToTensor you don't need to worry about guarding because that function is a well defined python function, so it might make the e2e test a bit cleaner.

gpetters94 · 2021-11-04T17:49:49Z

Yeah, that's definitely nicer. I'll refactor right now.

silvasean · 2021-11-04T18:36:48Z

I was looking into how this is implemented, and torch.ops is quite magical. See this docstring:

https://github.com/pytorch/pytorch/blob/master/torch/_ops.py#L32-L51

It works for any namespace! This means that any op that we cannot find, we can just use the JITOperator name to access it. In this case, you can do

😮 nice find!!! I didn't know it covered all non-c10 jit operators as well!

cathyzhyi · 2021-11-04T19:42:40Z

test/Conversion/TorchToLinalg/basic.mlir

+// CHECK:      %[[OUT:.*]] = torch_c.from_builtin_tensor %[[C]] : tensor<i64> -> !torch.vtensor<[],si64>
+// CHECK:      return %[[OUT]] : !torch.vtensor<[],si64>
+
+func @ntt(%arg0: !torch.int) -> !torch.vtensor<[],si64> {


sry that I missed this in the previous review. It's better to name the function torch.prim.NumToTensor.Scalar. ntt is not a common acronym.

cathyzhyi · 2021-11-04T19:43:19Z

test/Conversion/TorchToLinalg/basic.mlir

+// -----
+
+// CHECK:    func @ntt(%[[IN:.*]]: !torch.int) -> !torch.vtensor<[],si64> {
+// CHECK:      %[[A:.*]] = torch_c.to_i64 %[[IN]]


It's better to replace the A, B, C to more meaningful names like CST, INIT, FILL

ramiro050 · 2021-11-05T15:06:50Z

test/Dialect/Torch/refine-types.mlir

+// CHECK:    %[[CAST:.*]] = torch.tensor_static_info_cast %[[NTT]] : !torch.tensor<[],si64> to !torch.tensor
+// CHECK:    return %[[CAST]] : !torch.tensor
+
+func @aten_prim_numtotensor(%arg0: !torch.int) -> !torch.tensor {


The convention here is to name the function after the op, so @torch.prim.NumToTensor.Scalar. (note: the test matmul before this one breaks the convention)

@gpetters94 Could you address Ramiro's comment here? It would be great if you could fix the above tests as well. The alignments of CHECK are also off starting from aten_matmul_broadcast_matrix on line 982. could you help to fix that as well?

Sure. What do you mean by CHECK being misaligned? It looks fine to me.

I meant the space between CHECK tag and the texts to check against.

How does the formatting look in the most recent commit?

looks better! Thanks!

…ops (llvm#393) * Define krnl.permute op. * Support krnl.permute operation. * Properly remove loop references. * Re-push, Github was down. * Need to debug interpretOp error. * Fix lowering bug by erasing ops after full krnl IR interpretation is done, and clean up & comment code. * Introduce permute, unroll operations. * More debug. * Remove std::set. * krnl.terminate fails to be converted. * Pass all tests, need to add legal ops as well as part of the conversion target. * Change test format to new permute spec. * Bug fix for nested iterate op lowering. * Simplify error reporting. * Fix compilation error. * Increase comments coverage. * Remove unnecessary imports. * Re-trigger Jenkins * Add permute/unroll tests. * Retrigger Jenkins * simplified interface for gen code, plus start of gemm shape inference Signed-off-by: Alexandre Eichenberger <[email protected]> * gemm working but tested only for simple things Signed-off-by: Alexandre Eichenberger <[email protected]> * gemm with testing Signed-off-by: Alexandre Eichenberger <[email protected]> * added the store zero in the test and code Signed-off-by: Alexandre Eichenberger <[email protected]> * added the store zero in the test and code Signed-off-by: Alexandre Eichenberger <[email protected]> * re-enabled gemm for dyn tests Signed-off-by: Alexandre Eichenberger <[email protected]> * re-enable matmul dyn tests, currently 3d & 4d broken Signed-off-by: Alexandre Eichenberger <[email protected]> * first version that works for lowering Signed-off-by: Alexandre Eichenberger <[email protected]> * fixed bug and actualized the tests Signed-off-by: Alexandre Eichenberger <[email protected]> * enabled tests for runtime Signed-off-by: Alexandre Eichenberger <[email protected]> * removed debug Signed-off-by: Alexandre Eichenberger <[email protected]> Co-authored-by: Tian Jin <[email protected]>

gpetters94 force-pushed the numtotensor branch from c7586ce to 8507ce3 Compare November 2, 2021 07:41

pashu123 approved these changes Nov 2, 2021

View reviewed changes

cathyzhyi reviewed Nov 2, 2021

View reviewed changes

lib/Dialect/Torch/Transforms/RefineTypes.cpp Outdated Show resolved Hide resolved

cathyzhyi reviewed Nov 2, 2021

View reviewed changes

lib/Conversion/TorchToLinalg/TorchToLinalg.cpp Outdated Show resolved Hide resolved

ramiro050 reviewed Nov 2, 2021

View reviewed changes

lib/Dialect/Torch/Transforms/RefineTypes.cpp Outdated Show resolved Hide resolved

cathyzhyi reviewed Nov 2, 2021

View reviewed changes

lib/Dialect/Torch/Transforms/RefineTypes.cpp Outdated Show resolved Hide resolved

ramiro050 mentioned this pull request Nov 2, 2021

Add raw TorchScript->Linalg example #395

Closed

gpetters94 mentioned this pull request Nov 2, 2021

Push for BasicBertSequenceClassification_basic E2E #342

Closed

gpetters94 force-pushed the numtotensor branch from 8507ce3 to c971fc3 Compare November 3, 2021 00:12

cathyzhyi requested changes Nov 3, 2021

View reviewed changes

gpetters94 force-pushed the numtotensor branch 2 times, most recently from c3a5135 to 0e775f5 Compare November 4, 2021 05:51

gpetters94 requested a review from cathyzhyi November 4, 2021 06:08

cathyzhyi approved these changes Nov 4, 2021

View reviewed changes

ramiro050 reviewed Nov 4, 2021

View reviewed changes

gpetters94 force-pushed the numtotensor branch from 0e775f5 to 44d5bd1 Compare November 4, 2021 18:27

gpetters94 force-pushed the numtotensor branch from 44d5bd1 to 0c67ccd Compare November 4, 2021 18:52

cathyzhyi reviewed Nov 4, 2021

View reviewed changes

ramiro050 reviewed Nov 5, 2021

View reviewed changes

gpetters94 force-pushed the numtotensor branch 3 times, most recently from 4dc8879 to e7dc327 Compare November 8, 2021 20:28

Add NumToTensor

5b0d719

gpetters94 force-pushed the numtotensor branch from e7dc327 to 5b0d719 Compare November 8, 2021 20:50

cathyzhyi merged commit f419580 into llvm:main Nov 8, 2021

gpetters94 deleted the numtotensor branch November 8, 2021 20:58

Add NumToTensor #393

Add NumToTensor #393

Uh oh!

Conversation

gpetters94 commented Nov 2, 2021

Uh oh!

pashu123 left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

ramiro050 commented Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

silvasean commented Nov 2, 2021

Uh oh!

ramiro050 commented Nov 2, 2021

Uh oh!

silvasean commented Nov 2, 2021

Uh oh!

silvasean commented Nov 2, 2021

Uh oh!

silvasean commented Nov 2, 2021

Uh oh!

ramiro050 commented Nov 2, 2021

Uh oh!

silvasean commented Nov 2, 2021

Uh oh!

gpetters94 commented Nov 3, 2021

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!

ramiro050 commented Nov 3, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ramiro050 commented Nov 4, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ramiro050 commented Nov 4, 2021

Uh oh!

gpetters94 commented Nov 4, 2021

Uh oh!

silvasean commented Nov 4, 2021

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!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

cathyzhyi Nov 8, 2021 • 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!

Choose a reason for hiding this comment

ramiro050 commented Nov 2, 2021 •

edited

Loading

ramiro050 commented Nov 4, 2021 •

edited

Loading

cathyzhyi Nov 8, 2021 •

edited

Loading