[NNPA] Warning messages for legality check #2819

imaihal · 2024-05-08T15:13:32Z

Generate report on why the operations are not run on NNPA.
The report is displayed by using --opt-report=NNPAUnsupportedOps. The format is
==NNPA-UNSUPPORTEDOPS-REPORT==, <operation name>, <node name>, <reason>

  --opt-report=<value>                                                              - Provide information on a specific compiler optimization.
    =NoReport                                                                       -   No report. Default value.
    =Parallel                                                                       -   Provide report on how OMP Parallel is applied to ONNX ops.
    =Simd                                                                           -   Provide report on how SIMD is applied to ONNX ops.
    =NNPAUnsupportedOps

Example of xlm-roberta-base-language-detection-onnx (issue #2813):

$ onnx-mlir --O3 --mtriple=s390x-ibm-loz --mcpu=z16 --maccel=NNPA model.onnx  --EmitZHighIR --opt-report=NNPAUnsupportedOps

==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Mul, Mul_28, Element type is not F16 or F32.
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Add, Add_31, Element type is not F16 or F32.
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Div, Div_122, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Add, Add_125, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Mul, Mul_128, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Div, Div_216, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Add, Add_219, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Mul, Mul_222, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Div, Div_310, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Add, Add_313, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Mul, Mul_316, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Div, Div_404, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Add, Add_407, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].
==NNPA-UNSUPPORTEDOPS-REPORT==, onnx.Mul, Mul_410, Rank 0 is not supported. zAIU only supports rank in range of (0, 4].


:
:

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

tungld

LGTM! It's useful. Thanks for adding this!

tungld · 2024-05-09T01:16:01Z

src/Accelerators/NNPA/Conversion/ONNXToZHigh/ONNXLegalityCheck.cpp

+void emitLegalityCheckMessage(Operation *op, StringRef message) {
+  LLVM_DEBUG(llvm::outs() << "[NNPA Legality Check] Warning: " << op->getLoc()
+                          << " runs on CPU. Reason: " << message << "\n");
+}


Could you print out the operation name also? Sometime the loc is just a number and it's not easy to know which operation is.

Added operation name and node name. Thanks!

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

tungld · 2024-05-10T06:15:00Z

src/Accelerators/NNPA/Conversion/ONNXToZHigh/ONNXLegalityCheck.cpp

+      bool returnVal = (shapeA[1] == shapeB[0]);
+      if (!returnVal)
+        emitLegalityCheckMessage(
+            op.getOperation(), "Unstacked case, dim A 1 not equal dim B 0.");


Since we know the dimension size in this case, you can print out shapeA[1] and shapeB[0] as well, say Unstacked case, the 2nd dim of A (5) and the 1st dim of B (7) are not the same. if shapeA[1] =5 and shapeB[0] = 7`. Same for other static-shape cases.

Done. Thanks!

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

tungld · 2024-05-13T12:09:46Z

src/Accelerators/NNPA/Conversion/ONNXToZHigh/ONNXLegalityCheck.cpp

+    } else {
+      std::string message = "Element type is not F16 or F32.";
+      emitLegalityCheckMessage(op, StringRef(message));
+      return false;


I recommend to change emitLegalityCheckMessage to return false, e.g bool emitLegalityCheckMessage(Operation *op, StringRef message), and then we can write:

return emitLegalityCheckMessage(op, StringRef(message));

Done. Thanks. Also changed the function name.

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

tungld · 2024-05-15T05:24:58Z

src/Accelerators/NNPA/Conversion/ONNXToZHigh/ONNXLegalityCheck.cpp

-      return ((rank > 0) && (rank <= 4));
+      if ((rank > 0) && (rank <= 4)) {
+        return true;
+      } else {


Since we have return in the if part, the else is not necessary. For example:

if ((rank > 0) && (rank <= 4)) return true; std::string message = "Rank " + std::to_string(rank) + " is not supported. zAIU only supports rank in range of (0, 4]."; return emitWarningMessageNNPAUnsupported(op, StringRef(message));

Done. Thanks!

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

tungld · 2024-05-16T02:22:09Z

src/Accelerators/NNPA/Conversion/ONNXToZHigh/ONNXLegalityCheck.cpp

    }
  }
  return true;
 }

+bool emitWarningMessageNNPAUnsupported(Operation *op, StringRef message) {


Do we really need to use StringRef instead of just const std::string&? I don't see any operation on message and passing a pointer looks faster than constructing a StringRef.

Updated. Thanks!

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

tungld · 2024-05-20T08:26:40Z

src/Accelerators/NNPA/Conversion/ONNXToZHigh/RewriteONNXForZHigh.cpp

+            return !emitWarningMessageNNPAUnsupported(
+                op.getOperation(), message); /* true */
+
+          return false;


This can be shorten in this way:

return (!sameBatchDims) || emitWarningMessageNNPAUnsupported(op.getOperation(), message);

Updated. Thanks!

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal · 2024-05-23T06:31:59Z

@tungld I created an option for this reporting, not using --debug-only. This is the same approach as --opt-report option. Is this acceptable?

tungld · 2024-05-23T06:57:05Z

@tungld I created an option for this reporting, not using --debug-only. This is the same approach as --opt-report option. Is this acceptable?

If you take the approach of --opt-report, I recommend to add an enum value into the existing --opt-report, say --opt-report=NNPA. You can see --profile-ir to know how to mix CPU and NNPA values in a single option.

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal · 2024-05-24T04:42:25Z

If you take the approach of --opt-report, I recommend to add an enum value into the existing --opt-report,

I updated to use --opt-report adding new enum value (NNPAUnsupportedOps). Thanks!

tungld · 2024-05-24T06:38:17Z

I updated to use --opt-report adding new enum value (NNPAUnsupportedOps). Thanks!

When it's ready for another round of review, please let me know. Thanks!

imaihal · 2024-05-24T07:44:00Z

Yes. ready for review.

tungld

Thanks for updating the patch. It's much better. Just some minor comments, but the most important thing is updating the document. And, feel free to merge the patch then.

In your example of roberta, I see this

==NNPA-UNSUPPORTEDOPS-REPORT== onnx.Softmax, /roberta/encoder/layer.0/attention/self/Softmax_176, The rank of input tensor (4) needs to be 2 or 3.

It seemed it was not applied in this model. Do you know what was the reason? Softmax with rank 4 is potentially rewritten to rank 2 by RewriteONNXForZHigh pass. Perhaps axis was not the last dimension in this model ...

tungld · 2024-05-27T06:14:12Z

src/Accelerators/Accelerator.hpp

@@ -51,6 +51,10 @@

 #define ACCEL_PROFILEIR_CL_ENUM(name) PROFILEIR_CL_ENUM_##name

+#define ACCEL_OPTREPORT_ENUM(name) OPTREPORT_ENUM_##name
+
+#define ACCEL_OPTREPORT_CL_ENUM(name) OPTREPORT_CL_ENUM_##name


Could you update this document to include the new macro? Thanks!

Done. Thanks!

tungld · 2024-05-27T06:27:20Z

src/Accelerators/NNPA/Conversion/ONNXToZHigh/ONNXLegalityCheck.hpp

+bool emitWarningMessageNNPAUnsupported(
+    mlir::Operation *op, const std::string &message);
+
+bool emitWarningMessageInCompatibility(mlir::Operation *op);


Would you mind if we change these function names to make it consistent with functions for other --op-report options? We have onnxToKrnlSimdReport and onnxToKrnlParallelReport, so how about onnxToZHighUnsupportedReport? or feel free to choose another name.

Done. Thanks!

AlexandreEichenberger

Could you add a , after the option identifier, namely transforming

==NNPA-UNSUPPORTEDOPS-REPORT== onnx.Mul, /roberta/embeddings/Mul, Element type is not F16 or F32.

to

==NNPA-UNSUPPORTEDOPS-REPORT==,  onnx.Mul, /roberta/embeddings/Mul, Element type is not F16 or F32.

so that is is like all of the other perf measurements; it let you do CSV imports more easily.

Great addition, thanks.

AlexandreEichenberger

Approving the PR, but please add the missing comma in the printout to be compatible with the other tools.

imaihal · 2024-06-05T05:23:56Z

@tungld

In your example of roberta, I see this
==NNPA-UNSUPPORTEDOPS-REPORT== onnx.Softmax, /roberta/encoder/layer.0/attention/self/Softmax_176, The rank of input tensor (4) needs to be 2 or 3.
It seemed it was not applied in this model. Do you know what was the reason? Softmax with rank 4 is potentially rewritten to rank 2 by RewriteONNXForZHigh pass. Perhaps axis was not the last dimension in this model ...

I confirmed all softmax ops are lowered to zhigh.softmax in ZHighIR.

I found this message was generated in DevicePlacementPass which also calls legality check. In the DevicePlacementPass, is ONNXToZHighPass called after RewriteONNXForZHighPass? If not and ONNXToZHighPass checks original softmax op, I think this message is generated.

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal · 2024-06-05T08:21:25Z

Could you add a , after the option identifier, namely transforming

==NNPA-UNSUPPORTEDOPS-REPORT== onnx.Mul, /roberta/embeddings/Mul, Element type is not F16 or F32.

to

==NNPA-UNSUPPORTEDOPS-REPORT==,  onnx.Mul, /roberta/embeddings/Mul, Element type is not F16 or F32.

Done. Thanks!

tungld · 2024-06-05T13:46:41Z

I confirmed all softmax ops are lowered to zhigh.softmax in ZHighIR.

So the printed message was incorrect. I think you would fix that before merging.

is ONNXToZHighPass called after RewriteONNXForZHighPass?

Yes, it is in the code, but they are independent calls and used for analysis. The results of each call will be merged later. You can take a look at: https://github.com/onnx/onnx-mlir/blob/main/src/Accelerators/NNPA/Conversion/ONNXToZHigh/DevicePlacement.cpp#L196

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal · 2024-06-05T15:26:37Z

@tungld

Thanks for the comment!. I updated the PR to enable the reporting only in ONNXToZHighPass and disable it in DevicePlacementPass. This removes reporting about Softmax, as described in updated description of this PR.

imaihal added 3 commits May 8, 2024 15:05

Generate messages for legality check for MatMul op.

9fac96d

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

format

e98741b

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

format

66b0046

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal requested review from cjvolzka and tungld May 8, 2024 15:21

tungld approved these changes May 9, 2024

View reviewed changes

imaihal added 3 commits May 9, 2024 07:27

Update messages.

92d6451

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Add op name and node name in the message.

8223999

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into legality_check_msg

4f5d81f

tungld reviewed May 10, 2024

View reviewed changes

imaihal added 4 commits May 10, 2024 08:19

Merge branch 'main' into legality_check_msg

c83cebb

Adde messages for NNPA compatibility.

74b8cec

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Messages for verifying element type and rank.

64683b5

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Added all error messages for MatMul.

5b6b63a

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

tungld reviewed May 13, 2024

View reviewed changes

imaihal added 3 commits May 14, 2024 04:11

Update messages for rewriting for onnx-to-zhigh pass.

6541f3b

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into legality_check_msg

4fa1c9c

Add return value for function to emit messages.

b0822da

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

tungld reviewed May 15, 2024

View reviewed changes

imaihal added 2 commits May 16, 2024 02:01

Add warning messages for pooling ops.

0b3b4ba

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into legality_check_msg

a30cef2

tungld reviewed May 16, 2024

View reviewed changes

imaihal added 5 commits May 16, 2024 06:24

Use std::string for the function. Add more messages for other ops.

b0d7ca9

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Update messages for pooling.

86eed99

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Add warning message for reduceMean.

d9284eb

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Add messages for Pooling and Convolution ops.

943b165

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into legality_check_msg

8480696

tungld reviewed May 20, 2024

View reviewed changes

imaihal added 2 commits May 21, 2024 01:34

Revert update for pooling.

bb090a3

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Fix the check for pooling.

d3775e1

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal added 7 commits May 21, 2024 04:28

Merge branch 'main' into legality_check_msg

fcc111d

Add warning messages for LSTM and GRU.

59ecd94

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into legality_check_msg

9961e86

clean up the condition.

b1ed885

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Add an option for display report --nnpa-opt-report=NNPAUnsupportedOps

a6f1318

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Merge branch 'main' into legality_check_msg

cdf4c31

Apply change in this PR to softplus

005f5ef

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Add option in --opt-report and remove `--nnpa-opt-report

7b74612

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

imaihal requested a review from tungld May 24, 2024 07:43

tungld reviewed May 27, 2024

View reviewed changes

AlexandreEichenberger reviewed Jun 3, 2024

View reviewed changes

AlexandreEichenberger approved these changes Jun 3, 2024

View reviewed changes

Merge branch 'main' into legality_check_msg

30092eb

imaihal added 2 commits June 5, 2024 07:36

Add , in the messages.

eca8c83

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Update docs, rename functions.

1ef8db1

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Add a config parameter to disable reporting in DevicePlacementPass.

fe3dd87

Signed-off-by: Haruki Imai <imaihal@jp.ibm.com>

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[NNPA] Warning messages for legality check #2819

[NNPA] Warning messages for legality check #2819

imaihal commented May 8, 2024 •

edited

tungld left a comment

tungld May 9, 2024

imaihal May 9, 2024

tungld May 10, 2024

imaihal May 13, 2024

tungld May 13, 2024

imaihal May 14, 2024

tungld May 15, 2024

imaihal May 21, 2024

tungld May 16, 2024

imaihal May 21, 2024

tungld May 20, 2024

imaihal May 21, 2024

imaihal commented May 23, 2024

tungld commented May 23, 2024

imaihal commented May 24, 2024

tungld commented May 24, 2024

imaihal commented May 24, 2024

tungld left a comment

tungld May 27, 2024

imaihal Jun 5, 2024

tungld May 27, 2024

imaihal Jun 5, 2024

AlexandreEichenberger left a comment

AlexandreEichenberger left a comment

imaihal commented Jun 5, 2024 •

edited

imaihal commented Jun 5, 2024

tungld commented Jun 5, 2024

imaihal commented Jun 5, 2024

[NNPA] Warning messages for legality check #2819

Are you sure you want to change the base?

[NNPA] Warning messages for legality check #2819

Conversation

imaihal commented May 8, 2024 • edited

tungld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

imaihal commented May 23, 2024

tungld commented May 23, 2024

imaihal commented May 24, 2024

tungld commented May 24, 2024

imaihal commented May 24, 2024

tungld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

AlexandreEichenberger left a comment

Choose a reason for hiding this comment

imaihal commented Jun 5, 2024 • edited

imaihal commented Jun 5, 2024

tungld commented Jun 5, 2024

imaihal commented Jun 5, 2024

imaihal commented May 8, 2024 •

edited

imaihal commented Jun 5, 2024 •

edited