Add NNPA level compatibility check #2533

mikeessen · 2023-09-26T14:28:29Z

Add NNPA level compatibility check to validate operations for the specified --mcpu value.

Signed-off-by: Mike Essenmacher <[email protected]>

tungld

@mikeessen thanks for the patch!

I see we have another locations that need isCompatibleWithNNPALevel, those are inside addDynamicallyLegalOpFor functions in https://github.com/onnx/onnx-mlir/blob/main/src/Accelerators/NNPA/Conversion/ONNXToZHigh/RewriteONNXForZHigh.cpp.

tungld · 2023-09-27T02:15:53Z

src/Accelerators/NNPA/Conversion/ONNXToZHigh/NNPALimit.h

+// The NNPA levels.
+static constexpr const char *NNPA_Z13 = "z13";
+static constexpr const char *NNPA_Z14 = "z14";
+static constexpr const char *NNPA_Z15 = "z15";


Do we use NNPA_Z{13, 14, 15} in practice?

Thanks, I removed the definitions for NNPA_Z{13, 14, 15}

tungld · 2023-09-27T02:20:27Z

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

+/// A function to check whether the input NNPA level, ie. "z16", is compatible
+/// with the current NNPA level.
+bool isCompatibleWithNNPALevel(std::string inputNNPALevel) {
+  if (convertNNPALevel(inputNNPALevel) <= convertNNPALevel(mcpu)) {


For brevity, we can use return convertNNPALevel(inputNNPALevel) <= convertNNPALevel(mcpu);

BTW, a corner case, if both convertNNPALevel(inputNNPALevel) and convertNNPALevel(cpu) return 0 (because of some failure), this condition is true that is not what we want. Perhaps, you could check if convertNNPALevel succeeded or not before doing comparison.

Thanks, I updated for the corner case and also brevity.

tungld · 2023-09-27T02:21:19Z

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

+  // Check NNPA level.
+  if (!isCompatibleWithNNPALevel(NNPA_Z16)) {
+    return false;
+  }


Looks redundant. Just returning false for unsupported ops is enough.

Thanks, I updated to just return false.

tungld · 2023-09-27T02:22:01Z

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

+  // Check NNPA level.
+  if (!isCompatibleWithNNPALevel(NNPA_Z16)) {
+    return false;
+  }


Could you please remove { and } for this simple if? Same for other locations.

@tungld unless it's a standard in onnx-mlir to drop the {}'s for one line if statements, I lean towards keeping them. Functionally nothing changes but the braces are less error prone during maintenance or debugging scenarios.

@cjvolzka we don't have a standard but we follow LLVM coding style: https://llvm.org/docs/CodingStandards.html#don-t-use-braces-on-simple-single-statement-bodies-of-if-else-loop-statements

Ok, good to know. I defer to the established norm for the repo on such things.

Thanks, updated to remove { } from these statements.

The only places I think it's ok to have extra {} is in the presence of nested ifs, as dangling else are dangerous. Otherwise, best to keep to the LLVM formatting rules

Signed-off-by: Mike Essenmacher <[email protected]>

mikeessen · 2023-09-27T15:29:50Z

@tungld I updated the addDynamicallyLegalOpFor functions in https://github.com/onnx/onnx-mlir/blob/main/src/Accelerators/NNPA/Conversion/ONNXToZHigh/RewriteONNXForZHigh.cpp with isCompatibleWithNNPALevel. Please take a look.

tungld · 2023-09-28T03:38:41Z

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

@@ -499,6 +499,10 @@ void getRewriteONNXForZHighDynamicallyLegal(
  // broadcasting.
  addDynamicallyLegalOpFor<ONNXAddOp>(
      target, dimAnalysis, [](ONNXAddOp op, const DimAnalysis *dimAnalysis) {
+        // Check NNPA level.
+        if (!isCompatibleWithNNPALevel(NNPA_Z16)) {
+          return false;


@mikeessen I am sorry that the logic here is a bit confusing. false means the operation is illegal and must be rewritten to run on NNPA, while true means the operation is legal and will remain on CPU. So I think we should return true. Same for the other changes in this file.

It's contrary to the logic in isSuitableForZDNN. Actually it's like calling ! isSuitableForZDNN().

Thanks for the clarification. I updated false to true.

Signed-off-by: Mike Essenmacher <[email protected]>

tungld

LGTM! Thanks for the patch!

Signed-off-by: Mike Essenmacher <[email protected]>

jenkins-droid · 2023-10-03T15:42:43Z

Jenkins Linux ppc64le Build #11900 [push] Add NNPA level compatibi... started at 11:50

jenkins-droid · 2023-10-03T15:42:44Z

Jenkins Linux amd64 Build #12883 [push] Add NNPA level compatibi... started at 10:42

jenkins-droid · 2023-10-03T15:42:47Z

Jenkins Linux s390x Build #12907 [push] Add NNPA level compatibi... started at 11:42

jenkins-droid · 2023-10-03T16:59:28Z

Jenkins Linux amd64 Build #12883 [push] Add NNPA level compatibi... passed after 1 hr 16 min

jenkins-droid · 2023-10-03T17:04:40Z

Jenkins Linux s390x Build #12907 [push] Add NNPA level compatibi... passed after 1 hr 21 min

jenkins-droid · 2023-10-03T17:29:17Z

Jenkins Linux ppc64le Build #11900 [push] Add NNPA level compatibi... passed after 1 hr 46 min

AlexandreEichenberger · 2023-10-04T14:38:24Z

@mikeessen Can you confirm: we now need the -mcpu=z16 in order to have -maccel=NNPA take effect, or does the -maccel=NNPA grandfather the -mcpu=z16 option.

mikeessen · 2023-10-04T15:56:27Z

@AlexandreEichenberger Yes, mcpu z16 is required for maccel NNPA to take effect.

cjvolzka · 2023-10-04T16:05:31Z

we now need the -mcpu=z16 in order to have -maccel=NNPA take effect,

This is my expectation. Somewhere llvm has to be using something as a default mcpu when one isn't specified. Something makes me think that's z13 but I might be off. Whatever that default is, it's not z16. So an unspecified mcpu means "use z13 (or whatever the llvm default is) instructions". NNPA instructions won't be available for anything but the latest processor.

So before this change, specifying only -maccel=NNPA, we would create a model that targeted (by implied default) for z13 which couldn't possibly run on z13.

After this change, specifying only -maccel=NNPA, we compile a model that will run on z13 using all available NNPA instructions for z13 (which is zero).

AlexandreEichenberger · 2023-10-04T16:50:09Z

@mikeessen would you mind checking the doc that all our examples that have NNPA also have the -mcpu option? Tx

mikeessen · 2023-10-04T18:18:49Z

@AlexandreEichenberger Thanks, there are a couple examples which needed updating. I created a PR to address: #2551

AlexandreEichenberger · 2023-10-04T20:21:19Z

Thanks for the quick fixes on the doc :-)

mikeessen added 6 commits September 26, 2023 10:18

Add NNPA level compatability check

774823f

Signed-off-by: Mike Essenmacher <[email protected]>

Clang format changes

0ed10b5

Signed-off-by: Mike Essenmacher <[email protected]>

Clang format changes

701253e

Signed-off-by: Mike Essenmacher <[email protected]>

Add mcpu to NNPA tests

fda058c

Signed-off-by: Mike Essenmacher <[email protected]>

Add link dependency to ONNXToZHigh for OMCompilerOptions

d9db200

Signed-off-by: Mike Essenmacher <[email protected]>

Update link dependency

efd15e3

Signed-off-by: Mike Essenmacher <[email protected]>

mikeessen requested review from tungld and cjvolzka September 26, 2023 22:02

tungld reviewed Sep 27, 2023

View reviewed changes

Review updates

0de1f8d

Signed-off-by: Mike Essenmacher <[email protected]>

tungld reviewed Sep 28, 2023

View reviewed changes

Review updates

8ea52c8

Signed-off-by: Mike Essenmacher <[email protected]>

tungld approved these changes Oct 2, 2023

View reviewed changes

mikeessen and others added 3 commits October 3, 2023 09:34

Resolve conflicts

456ef1b

Signed-off-by: Mike Essenmacher <[email protected]>

Merge branch 'main' into essen-updates

50029cf

Signed-off-by: Mike Essenmacher <[email protected]>

Add mcpu z16 to device_placement_pass_perf_model.mlir

79e17a9

Signed-off-by: Mike Essenmacher <[email protected]>

mikeessen merged commit 7592edf into onnx:main Oct 3, 2023
8 checks passed

mikeessen deleted the essen-updates branch October 4, 2023 17:19

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Add NNPA level compatibility check #2533

Add NNPA level compatibility check #2533

mikeessen commented Sep 26, 2023

tungld left a comment

tungld Sep 27, 2023

mikeessen Sep 27, 2023

tungld Sep 27, 2023

mikeessen Sep 27, 2023

tungld Sep 27, 2023

mikeessen Sep 27, 2023

tungld Sep 27, 2023

cjvolzka Sep 27, 2023

tungld Sep 28, 2023

cjvolzka Sep 28, 2023

mikeessen Sep 28, 2023

AlexandreEichenberger Sep 28, 2023

mikeessen commented Sep 27, 2023

tungld Sep 28, 2023

mikeessen Sep 28, 2023

tungld left a comment

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

AlexandreEichenberger commented Oct 4, 2023

mikeessen commented Oct 4, 2023

cjvolzka commented Oct 4, 2023

AlexandreEichenberger commented Oct 4, 2023

mikeessen commented Oct 4, 2023

AlexandreEichenberger commented Oct 4, 2023

Add NNPA level compatibility check #2533

Add NNPA level compatibility check #2533

Conversation

mikeessen commented Sep 26, 2023

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

mikeessen commented Sep 27, 2023

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tungld left a comment

Choose a reason for hiding this comment

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

jenkins-droid commented Oct 3, 2023

AlexandreEichenberger commented Oct 4, 2023

mikeessen commented Oct 4, 2023

cjvolzka commented Oct 4, 2023

AlexandreEichenberger commented Oct 4, 2023

mikeessen commented Oct 4, 2023

AlexandreEichenberger commented Oct 4, 2023