<table border="1" cellspacing="0" cellpadding="8">
    <tr>
        <th>Issue</th>
        <td>
            <a href=https://github.com/llvm/llvm-project/issues/67797>67797</a>
        </td>
    </tr>

    <tr>
        <th>Summary</th>
        <td>
            MLIR: `SymbolUserOpInterface` may fail with `PatternRewrite`s of symbol-defining ops
        </td>
    </tr>

    <tr>
      <th>Labels</th>
      <td>
            mlir
      </td>
    </tr>

    <tr>
      <th>Assignees</th>
      <td>
      </td>
    </tr>

    <tr>
      <th>Reporter</th>
      <td>
          mortbopet
      </td>
    </tr>
</table>

<pre>
    A bit of an intricate pattern, but has come up surprisingly often over in CIRCT. In cases where
one runs a pass using a pattern rewriter with `--debug` enabled, the pass will crash during the intermediate verification that is being done when printing the IR using `--debug` if
1. symbol-defining container-like operations are being replaced at the `mlir::ModuleOp`-level
2. symbol-using ops are being replaced
3. those symbol-using ops use a symbol table and inspect it at the `mlir::ModuleLevel` in their `verifySymbolUses` functions.
4. The pass runs on the `mlir::ModuleOp` level
  
To me, this seems like a reasonable and legal use (especially given that the IR reaches a legal state after pattern application).

Toy example:

Say we're doing a conversion from container ops `op1` to `op2` and here we have operations that reference those container ops, `userA` and `userB`

```tablegen
class BaseContainer<string mnemonic> : Op<MyDialect, mnemonic,
  [Symbol, SymbolTable, NoTerminator, SingleBlock]> {
    let regions = (region SizedRegion<1>:$body);
    let arguments = (ins SymbolNameAttr:$sym_name);
    let assemblyFormat = "$sym_name attr-dict-with-keyword $body";
}

def Op1 :BaseContainer<"op1"> {}
def Op2 :BaseContainer<"op2"> {}

class BaseUser<string mnemonic> : Op<MyDialect, mnemonic, [
 DeclareOpInterfaceMethods<SymbolUserOpInterface>,
]> {
    let arguments = (ins FlatSymbolRefAttr:$sym_name);
    let assemblyFormat = "$sym_name attr-dict";

    let extraClassDefinition = [{
 LogicalResult $cppClass::verifySymbolUses(SymbolTableCollection &symbolTable) {
        auto mod = (*this)->getParentOfType<mlir::ModuleOp>();
 return success(symbolTable.lookupSymbolIn(mod, getSymNameAttr()));
 }
    }];
}

def UserA : BaseUser<"userA">{}
def UserB : BaseUser<"userB">{}
```

And define conversion patterns which simply translates one op to the other:
```C++
struct OneToTwoPattern : public OpConversionPattern<Op1> {
  using OpConversionPattern<Op1>::OpConversionPattern;
  using OpAdaptor = Op1::Adaptor;

  LogicalResult
  matchAndRewrite(Op1 op, OpAdaptor adaptor,
                  ConversionPatternRewriter &rewriter) const override {
    auto newOp = rewriter.create<Op2>(op.getLoc(), op.getSymName());
    newOp.getRegion().push_back(new Block);

 // Move ops from Op1 to Op2
 rewriter.mergeBlocks(&op.getRegion().front(), &newOp.getRegion().front(),
 {});

    // The issue is triggered when all ops of the `mlir::ModuleOp` pass
    // verification. To not complicate this example too much, we just add a
    // dummy block here to make the module pass verification and trigger the
    // bug.
 op.getRegion().push_back(new Block);
    rewriter.eraseOp(op);
 return success();
  }
};

struct AToBPattern : public OpConversionPattern<UserA> {
  using OpConversionPattern<UserA>::OpConversionPattern;
  using OpAdaptor = UserA::Adaptor;

  LogicalResult
  matchAndRewrite(UserA op, OpAdaptor adaptor,
                  ConversionPatternRewriter &rewriter) const override {
    rewriter.replaceOpWithNewOp<UserB>(op, op.getSymName());
    return success();
  }
};
```

With an example program:
```mlir
mydialect.op1 @foo {}
mydialect.op1 @bar{
   mydialect.userA @foo
}
```


Now, running this without `--debug`, everything goes fine:
```mlir
module {
 mydialect.op2 @foo {
  }
  mydialect.op2 @bar {
    mydialect.userB @foo
  }
}
```

However, adding `--debug`, we get the following debug output, and error message:
```
//===-------------------------------------------===//
Legalizing operation : 'builtin.module'(0xe1cdc0) {
  * Fold {
ImplicitTypeIDRegistry::lookupOrInsert(mlir::OpTrait::ConstantLike<Empty>)
ImplicitTypeIDRegistry::lookupOrInsert(mlir::DialectFoldInterface)
 } -> FAILURE : unable to fold
} -> FAILURE : no matched legalization pattern
//===-------------------------------------------===//

//===-------------------------------------------===//
Legalizing operation : 'mydialect.op1'(0xe22230) {
  * Fold {
  } -> FAILURE : unable to fold

  * Pattern : 'mydialect.op1 -> ()' {
Trying to match "(anonymous namespace)::OneToTwoPattern"
    ** Insert  : 'mydialect.op2'(0xe2cf80)
    ** Erase   : 'mydialect.op1'(0xe22230)
"(anonymous namespace)::OneToTwoPattern" result 1

    //===-------------------------------------------===//
 Legalizing operation : 'mydialect.op2'(0xe2cf80) {
    } -> SUCCESS : operation marked legal by the target
 //===-------------------------------------------===//
  } -> SUCCESS : pattern applied successfully
// *** IR Dump After Pattern Application ***
mlir-asm-printer: Verifying operation: builtin.module
circt-opt: llvm/mlir/lib/IR/SymbolTable.cpp:143: mlir::SymbolTable::SymbolTable(mlir::Operation *): Assertion `inserted.second && "expected region to contain uniquely named symbol operations"' failed.
```

The error message now occurs when IR verification is being run prior to printing the IR, with the error trace being:
1. top-level `module` op is verified
2. `mydialect.op2` is verified. Since this is a symbol table, [it will also verify its symbol user ops](https://github.com/llvm/llvm-project/blob/a806aef7535075a1fa2e2159803c9dceb042b6ff/mlir/lib/IR/SymbolTable.cpp#L445)
4. This calls `verifySymbolUses` of  `mydialect.user1`, which in turn inspects the symbol table of the top-level module op
5. This prompts symbol table generation to be performed on the module. Since `op1` technically hasn't been erased yet, there's now duplicate symbols in the symbol table, and the assert is fired.

To me, the simplest fix (which would also fix other debug-print related issues we've seen) would be to never assume that the IR is in a valid form when printing the IR during pattern rewriting - that is, always print the entire IR in generic form when debug printing during pattern rewriting.
Whether right or not, there are many places (such as the one above) where code paths assume that the surrounding IR state is valid, such as in printers and verifiers... so maybe the overall question that should be asked here is whether or not we can safely run IR verification at any time during pattern rewriting - if the above is indeed seen as a legal use of the pattern rewrite infrastructure, the answer may be no.

</pre>
<img width="1px" height="1px" alt="" src="http://email.email.llvm.org/o/eJzEWVtz2zqS_jXwS5dVFGjdHvwgy3Gtq5x4ynF2HqdAsilhDAI8AGhF59dvNcCLKMk5ydnsjsuVmCTQ6OvXjW7hnNxqxFs2u2Oz-yvR-J2xt5WxPjM1-qvMFIfbNWTSgylBaJDaW5kLj1AL79FqxjeQNR52wkFuKoSmBtfY2kon9VYdwJQeNZh3tCA1bB5fNq8TeNSQC4cO9ju0yJJ7lqyNRrCNdiCgFs5BQwTCQzgILO6t9GhhL_0O2Dy5vi4wa7ZsngBqkSksiBm_w7h_L5WC3Aq3g6KxRIs-Se3RVlhIkuEdrSxJHGk0-J3wIB1kSGsLYme_Qw21ldp32x9fWr7G58syyjCdgDtUmVHXBZZS08LcaC-kRnut5BuCqdGG8xwIi-1hFmslcixA-HAKmyeVkpala5auP5uiUfhc04kK31HFo3h_VGTI1JcoxrXpBPzOODzf0TgE0b4GT0oEoQuQ2tWYe5D-Byw9BWZIelIeSkuLgkoPXwPBbw4dfS8bnQeRJ5Gdmwm8dlYKFg_a_1hsOBIbIP7_aqDCaG_pwCFWDoKCBVgUzuheFoVboYKgjC-R5JJCqQNs5Tu2Vm8ta1HkOyQHjHucJycRJTld54WirlXrMYyvWoE6lg6A30VVKyQRjr58FQfYI-MLi1CY6Na50e9oHXleaU01-EmwC5snpp6S7N7EB04PJA9FDOwRduJ95E5BEoslWtQ5tgYfUSV1sXnSOLTrjlr7fMfmyTHH9Bh-g09sUcfXuSKb3QmHm44wSzfOh_CqNFZGy5yln4Cla3iuWbr5fLiXQmHu6fB-Bd_01pzdRWeh7_GvVzqSHr-YV7SV1MIbGz4TouCdMvkbm92HYxZ3HSEAhST_NiiDpfdk7vgIX-WfWLyEv1m6mbL0ExmI3xC8Mb5i6QkVYbdNhdr3dKR2LXNfRIVr720k4A7Vv7QgR7xAxDmsMnV4MLYSvqXEjzaB8N5eFzL31wRp12942BtbQM8Y72myxf2xdQos4bmekpZPbcE4J8ehvVE93c64h3-4h1_Yc2r0b-5v25sM3ernHnMlLD7Xj4TFpcjxM_qdKRxLNz1w2KPPZK_OYz4y_EWTPSjhI8UXLH-71UYGGhHB796KDantPqSBkF8CqdndwPmT2cpcqBd0jfJk9byuw6YIgGdIypdH8bExirQcCPO5Ow6c1Vg79CMab6AyRaccxteEm4yvrln6aYv-H8Ki9s_l66FGlm4uwDDZYDnSmEXfWA2uyXN0xN4RFxNlzFtTR4YfNePLyoT0vEWySB9FkWT87Qn3_kes0wPZ_ONIIG9ZBw888lHGecS54NWngUCL7j7Ycne-pUfD46PXuoCQ5fEYy9s8QZWNzHfgZFWrA3grtFPCI-U6Qm2Cdco6xu_QDtmiO2fD-B39hrfO2yb38Kzx1bzuzT_aTETc102mZA7P9abnoP3M0s1zPT0JlZj3f7Q6Gv3iijMq60LU3tjgU2E3bW1fngXGyNm7l5Xw-W6ti5dY2jG-JFQzNTnKQF-0JIekcfpzxu1LVysyPu_qRgqM3GjnQzFqZYHjOAkxonH_XAeJum2T3KLwGDTEYxSYerJF_2Tyzn03EF-1nj149TG6BNK0qk1EYdGkbtzuX5nI3xhfatxDzG5HW7uY4A-MP8BnE5K-izUDacsbgvU-JFumK7TbmCldOGhuzo8urdF-EIHx-WUWx-v6GA3hcc4ohWzklWo86VxD_4K3crtFi0UsqoVSQQxT_rjyoxrxjPBx2T6BVwPaeLp8xMoMY0nYVmLgjYGqyXck4h7h343zIIoCxBnZoqmqA2SktFhkEWaKNwwcVoGpWLOOrg1URLXC0cIzqlmzbatEuGCDvzQ_UeqtilY4Ugy54I-heExiALLF_Ym9WnBZv5q7nwaWALi_AC3d-r8LLu3-3wAvMVX8hwCmN2N7OXuu_yn97gvFXKukuw5ffhJS_pbZL-UyYoSu913Q1NZsrajOE1OI0vCqOhSx0JsYqkRvktKYUfF4tiAT9lgfw_cmJvBA4jTLX-I2_vvF7ElNttE63s6lC60B0_jx7ZxW4Tvag9_Rwq1BB5S2fyhdDPeB32Np-LG4p7qG86WZsGNPGIt-NxL9xGwfa-G_zJ6kIulEUZz1JFq822K83pZGKbMPnQ1aAKbxdROKdAIwtNZYqNA5sb2gl_YxABpL7-Pv9c__9HtaEoHcE92x5Z-xE9HeYgP2ML7IGqm81JNoBsYXjC-T7zjNizw5KXAZX8ODUcXw7jEkAumpln28J7h13h4ifsSy9Nk-aoeWktqQdp7rVyukjw8bimOh_ZN8o9T_qar9IQTn6n91Rns3In6H201HkwwPVI_Dw_rx6dtLvFU1sZPhDZmw6B3jfKE2EfCwbXnIP6NKu0bd_4UN_98dYwQqvV9wztO_9gv4BQUfETlOi6ccRGodOC-Gs17tIYBSa5N4h1wKbfShMo0Dukm6urV-dL9xeU8bjkqJNXESHQouccIHXeTlMhl8atj9iYoHuLj7XJOdYX-da7DxPju9XBX-Ni-Bn3WTc9WMwbh3iq_fNptPX78GGgPFSti3LqQgOwQw9cJu0Y8q898n12WGRv1HLLqcXzZKHY6jsLV2cJcXuG-qGtahgdm58XpoYA5r26SnpL0WrroOLe9wL4X_Dl2IkZLp9QlAxz6RtLm_NjVBKCj1XjH-EKCPPyiZMf7w-ML4w1EPY5LXNUvX05uUdgwoedwGPHsxhuze7iQG-SSsHQVJeDdPZIgYLCYOc6MLqtYYn1M04vcac49F2zGkSG17pdBo-UeD6hDcveia40OnNUTFAkohFRaTH2RougCNMitosweT54118Rr0-DK-TPTjB9uEwYOxxNjJBCJkdirYfE_fW5G3rf8-e08n4E0dRwbhghVNNU_A1HRQPLgbEfBJWDOKm3lyvG4CX2VsLEtH78djg7bFJ32cuwjlTNx5AOldt5TKndCJnt0zvtx5X4dmV_DcrfS7JpvkhvymdR_677q25t-hofiQKUN-JJbJXGC5mKWzZDET01Jw5NPZapmk-arIMUtueDYvy5_yP54-3dzMesgLswnpIBdKuY8GGqaEsbZIrmlXdIXGj9QQavN2kuKCsUZzlvbqO9iorTlNHTmZtZzU1lT1oMK4eYu6c31vIEOo0ZbGkr-2w5RIrTPa0TwB852mu5I6wE44zfjCQ4aoIdwuCzigb-doliovF5y2aLrLdWTDtUOfMx8I1-Edhj6qDfO0UtohTGJcDLMbjB0ydB5K-Z0yaVTf3jSqiF5E70ObLFauEZzAohIUvqHB4OJ05R3BIWpC-Lg_C2ldU5lMDDUVjuY9Mkgh4F0oWQCp7_LErx0fjgeR9Oa6GxoGydVeHFzcHENTe2njQToaTOZHp8Q6vD_ro0Nazf1zh0EHVm53HowFbQYzhcFfJfQBwsXSkR5dk-9ARL8zGkFk5j00h8PIFXJThBnuzp1pxjXWmkaHK8XjSzsFIyAgNdGZHWnZqgqtC3ZvkcK6yWQCjkqfQxa7J3QhFkrBHw26Ydjqdp2VhKMcGxiTARqDrFFKusXkQoMTJYEyIeMpbAoPJLyXFf7IWDJGXNBENH6BBPDk_GIY-zWuD86T2TNIXVoRWyaN7V1YaLdHS-KSLNq0JrsqbtNila7EFd5O56vZajWdTpOr3W0xXd4IvOHJfJ6ls4Ln83mSlnyRJPNlOU_mV_KWJzxNVnw15ekyTSaLGxTpajYv07IokixlNwlWQqoJAeTE2O1ViIPb-WKxWlwpkaFyYajPeURAzmb3V_Y24GnWbB27SZR03g0EvPQKbz8_Pb6EGmqeXB7FzJMgJyW_fhA_7omweRL6eqeDcFO7q8aq219G_RjijD8E6f4nAAD__822IGU">