circt icon indicating copy to clipboard operation
circt copied to clipboard

[Caylx] canonicalization results in invalid IR: ifOp w/empty else

Open dtzSiFive opened this issue 1 year ago • 1 comments

Verifier checks that else region is non-empty if present.

Current patterns can fail this verification, for example when the else region contains some control op that removes itself (emptyControl<OpTy>) the containing block is now newly empty and fails verification.

Caught by -DMLIR_ENABLE_EXPENSIVE_PATTERN_API_CHECKS=ON..

cc #7047.

This can be observed with the following example from test/Dialect/Calyx/canonicalization.mlir:

module attributes {calyx.entrypoint = "main"} {
  calyx.component @main(%go: i1 {go}, %clk: i1 {clk}, %reset: i1 {reset}) -> (%done: i1 {done}) {
    %r.in, %r.write_en, %r.clk, %r.reset, %r.out, %r.done = calyx.register @r : i1, i1, i1, i1, i1, i1
    %eq.left, %eq.right, %eq.out = calyx.std_eq @eq : i1, i1, i1
    %c1_1 = hw.constant 1 : i1
    calyx.wires {
      calyx.comb_group @Cond {
        calyx.assign %eq.left =  %c1_1 : i1
        calyx.assign %eq.right = %c1_1 : i1
      }
      calyx.group @A {
        calyx.assign %r.in = %c1_1 : i1
        calyx.assign %r.write_en = %c1_1 : i1
        calyx.group_done %r.done : i1
      }
    }
    calyx.control {
      calyx.seq {
        calyx.if %eq.out with @Cond {
          calyx.seq {
            calyx.enable @A
          }
        } else {
          calyx.seq {
            calyx.enable @A
          }
        }
      }
    }
  }
}

dtzSiFive avatar May 16 '24 16:05 dtzSiFive

There are other similar occurrences, such as:

+ /build/sifive/expensive-abi/bin/circt-opt /home/will/src/sifive/circt/test/Conversion/LoopScheduleToCalyx/convert_pipeline.mlir -lower-loopschedule-to-calyx -split-input-file                                                                                                                                                                                                                                                                                                                                                                    |  ¦-foldMod(Op op,ArrayRef<Attribute> co
within split at /home/will/src/sifive/circt/test/Conversion/LoopScheduleToCalyx/convert_pipeline.mlir:1 offset :37:1: error: 'calyx.component' op The component currently does nothing. It needs to either have continuous assignments in the Wires region or control constructs in the Control region.                                                                                                                                                                                                                                             |  ¦-foldMuxChain(MuxOp rootMux,bool isFa
func.func @minimal() {                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |  ¦-foldMuxOfUniformArrays(MuxOp op,Patt
^                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   |  ¦-getCommonOperand(Op op)
within split at /home/will/src/sifive/circt/test/Conversion/LoopScheduleToCalyx/convert_pipeline.mlir:1 offset :37:1: note: see current operation:·                                                                                                                                                                                                                                                                                                                                                                                                 |  ¦-getConcatOperands(Value v,SmallVecto
"calyx.component"() ({                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |  ¦-getIntAttr(const APInt & value,MLIRC
^bb0(%arg0: i1, %arg1: i1, %arg2: i1, %arg3: i1):                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                   |  ¦-getLowestBitAndHighestBitRequired(Op
  "calyx.wires"() ({                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                |  ¦-getMuxChainCondConstant(Value cond,V
  ^bb0:                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             |  ¦-getTotalWidth(ArrayRef<Value> operan
  }) : () -> ()                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     |  ¦-hasOperandsOutsideOfBlock(Operation
  "calyx.control"() ({                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                              |  ¦-hasSVAttributes(Operation * op)
  ^bb0:                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                             |  ¦-m_Complement(const SubType & subExpr
  }) : () -> ()                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                     |  ¦-matchAndRewriteCompareConcat(ICmpOp
}) {function_type = (i1, i1, i1, i1) -> (), portAttributes = [{clk}, {reset}, {go}, {done}], portDirections = -8 : i4, portNames = ["clk", "reset", "go", "done"], sym_name = "minimal", toplevel} : () -> ()                                                                                                                                                                                                                                                                                                                                       |  ¦-narrowOperationWidth(OpTy op,bool na
LLVM ERROR: IR failed to verify after pattern application                                                                                                                                                                                                                                                                                                                                                                                                                                                                                           |  ¦-replaceOpAndCopyName(PatternRewriter
PLEASE submit a bug report to https://github.com/llvm/circt and include the crash backtrace.    

Probably the verifier is meant to ensure --after all canonicalizers run-- that no useless things are left. Hmm.

dtzSiFive avatar May 16 '24 18:05 dtzSiFive