From acab6a70fb40cc7d363bee047ef0a250416b77ad Mon Sep 17 00:00:00 2001 From: Mehdi Amini Date: Mon, 13 Mar 2023 17:49:23 +0100 Subject: [PATCH] Revert "Add a `skipRegion()` feature to the OpPrintingFlags for MLIR ASM printer" This reverts commit 0fe16607a523af3d8978ad636134e4d3034e365c which wasn't ready to land. --- mlir/include/mlir/IR/OperationSupport.h | 9 ------ mlir/lib/IR/AsmPrinter.cpp | 42 ++++++--------------------- mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp | 19 +++++++++--- mlir/test/mlir-lsp-server/hover.test | 4 +-- 4 files changed, 26 insertions(+), 48 deletions(-) diff --git a/mlir/include/mlir/IR/OperationSupport.h b/mlir/include/mlir/IR/OperationSupport.h index ba8f0a8..ebeb0a9 100644 --- a/mlir/include/mlir/IR/OperationSupport.h +++ b/mlir/include/mlir/IR/OperationSupport.h @@ -834,9 +834,6 @@ public: /// Always print operations in the generic form. OpPrintingFlags &printGenericOpForm(); - /// Skip printing regions. - OpPrintingFlags &skipRegions(); - /// Do not verify the operation when using custom operation printers. OpPrintingFlags &assumeVerified(); @@ -864,9 +861,6 @@ public: /// Return if operations should be printed in the generic form. bool shouldPrintGenericOpForm() const; - /// Return if regions should be skipped. - bool shouldSkipRegions() const; - /// Return if operation verification should be skipped. bool shouldAssumeVerified() const; @@ -888,9 +882,6 @@ private: /// Print operations in the generic form. bool printGenericOpFormFlag : 1; - /// Always skip Regions. - bool skipRegionsFlag : 1; - /// Skip operation verification. bool assumeVerifiedFlag : 1; diff --git a/mlir/lib/IR/AsmPrinter.cpp b/mlir/lib/IR/AsmPrinter.cpp index f4d6654..8c5bb30 100644 --- a/mlir/lib/IR/AsmPrinter.cpp +++ b/mlir/lib/IR/AsmPrinter.cpp @@ -183,9 +183,8 @@ void mlir::registerAsmPrinterCLOptions() { /// Initialize the printing flags with default supplied by the cl::opts above. OpPrintingFlags::OpPrintingFlags() : printDebugInfoFlag(false), printDebugInfoPrettyFormFlag(false), - printGenericOpFormFlag(false), skipRegionsFlag(false), - assumeVerifiedFlag(false), printLocalScope(false), - printValueUsersFlag(false) { + printGenericOpFormFlag(false), assumeVerifiedFlag(false), + printLocalScope(false), printValueUsersFlag(false) { // Initialize based upon command line options, if they are available. if (!clOptions.isConstructed()) return; @@ -224,12 +223,6 @@ OpPrintingFlags &OpPrintingFlags::printGenericOpForm() { return *this; } -/// Always skip Regions. -OpPrintingFlags &OpPrintingFlags::skipRegions() { - skipRegionsFlag = true; - return *this; -} - /// Do not verify the operation when using custom operation printers. OpPrintingFlags &OpPrintingFlags::assumeVerified() { assumeVerifiedFlag = true; @@ -277,9 +270,6 @@ bool OpPrintingFlags::shouldPrintGenericOpForm() const { return printGenericOpFormFlag; } -/// Return if Region should be skipped. -bool OpPrintingFlags::shouldSkipRegions() const { return skipRegionsFlag; } - /// Return if operation verification should be skipped. bool OpPrintingFlags::shouldAssumeVerified() const { return assumeVerifiedFlag; @@ -624,11 +614,9 @@ private: /// Print the given operation in the generic form. void printGenericOp(Operation *op, bool printOpName = true) override { // Consider nested operations for aliases. - if (!printerFlags.shouldSkipRegions()) { - for (Region ®ion : op->getRegions()) - printRegion(region, /*printEntryBlockArgs=*/true, - /*printBlockTerminators=*/true); - } + for (Region ®ion : op->getRegions()) + printRegion(region, /*printEntryBlockArgs=*/true, + /*printBlockTerminators=*/true); // Visit all the types used in the operation. for (Type type : op->getOperandTypes()) @@ -677,10 +665,6 @@ private: bool printEmptyBlock = false) override { if (region.empty()) return; - if (printerFlags.shouldSkipRegions()) { - os << "{/*skip region*/}"; - return; - } auto *entryBlock = ®ion.front(); print(entryBlock, printEntryBlockArgs, printBlockTerminators); @@ -3357,14 +3341,10 @@ void OperationPrinter::printGenericOp(Operation *op, bool printOpName) { // Print regions. if (op->getNumRegions() != 0) { os << " ("; - if (!printerFlags.shouldSkipRegions()) { - interleaveComma(op->getRegions(), [&](Region ®ion) { - printRegion(region, /*printEntryBlockArgs=*/true, - /*printBlockTerminators=*/true, /*printEmptyBlock=*/true); - }); - } else { - os << "/*skip " << op->getNumRegions() << " regions*/"; - } + interleaveComma(op->getRegions(), [&](Region ®ion) { + printRegion(region, /*printEntryBlockArgs=*/true, + /*printBlockTerminators=*/true, /*printEmptyBlock=*/true); + }); os << ')'; } @@ -3483,10 +3463,6 @@ void OperationPrinter::printSuccessorAndUseList(Block *successor, void OperationPrinter::printRegion(Region ®ion, bool printEntryBlockArgs, bool printBlockTerminators, bool printEmptyBlock) { - if (printerFlags.shouldSkipRegions()) { - os << "{/*skip region*/}"; - return; - } os << "{" << newLine; if (!region.empty()) { auto restoreDefaultDialect = diff --git a/mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp b/mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp index 8597423..397f259 100644 --- a/mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp +++ b/mlir/lib/Tools/mlir-lsp-server/MLIRServer.cpp @@ -516,12 +516,23 @@ std::optional MLIRDocument::buildHoverForOperation( os << "Generic Form:\n\n```mlir\n"; - op.op->print(os, OpPrintingFlags() - .printGenericOpForm() - .elideLargeElementsAttrs() - .skipRegions()); + // Temporary drop the regions of this operation so that they don't get + // printed in the output. This helps keeps the size of the output hover + // small. + SmallVector> regions; + for (Region ®ion : op.op->getRegions()) { + regions.emplace_back(std::make_unique()); + regions.back()->takeBody(region); + } + + op.op->print( + os, OpPrintingFlags().printGenericOpForm().elideLargeElementsAttrs()); os << "\n```\n"; + // Move the regions back to the current operation. + for (Region ®ion : op.op->getRegions()) + region.takeBody(*regions.back()); + return hover; } diff --git a/mlir/test/mlir-lsp-server/hover.test b/mlir/test/mlir-lsp-server/hover.test index 4fb9856..df0fe20 100644 --- a/mlir/test/mlir-lsp-server/hover.test +++ b/mlir/test/mlir-lsp-server/hover.test @@ -114,7 +114,7 @@ // CHECK-NEXT: "result": { // CHECK-NEXT: "contents": { // CHECK-NEXT: "kind": "markdown", -// CHECK-NEXT: "value": "\"func.func\" : public @foo\n\nGeneric Form:\n\n```mlir\n\"func.func\"() (/*skip 1 regions*/) {function_type = (i1) -> (), sym_name = \"foo\"} : () -> ()\n```\n" +// CHECK-NEXT: "value": "\"func.func\" : public @foo\n\nGeneric Form:\n\n```mlir\n\"func.func\"() ({\n}) {function_type = (i1) -> (), sym_name = \"foo\"} : () -> ()\n```\n" // CHECK-NEXT: }, // CHECK-NEXT: "range": { // CHECK-NEXT: "end": { @@ -138,7 +138,7 @@ // CHECK-NEXT: "result": { // CHECK-NEXT: "contents": { // CHECK-NEXT: "kind": "markdown", -// CHECK-NEXT: "value": "\"func.func\" : public @foo\n\nGeneric Form:\n\n```mlir\n\"func.func\"() (/*skip 1 regions*/) {function_type = (i1) -> (), sym_name = \"foo\"} : () -> ()\n```\n" +// CHECK-NEXT: "value": "\"func.func\" : public @foo\n\nGeneric Form:\n\n```mlir\n\"func.func\"() ({\n}) {function_type = (i1) -> (), sym_name = \"foo\"} : () -> ()\n```\n" // CHECK-NEXT: }, // CHECK-NEXT: "range": { // CHECK-NEXT: "end": { -- 2.7.4