Backport parentheses fix to 1.5.x (#7121)

* [ExportVerilog] Emit extra parentheses around reduction ops (#3357)

This closes #3001. This PR is a follow-up onto
reduction operators appear in the head of subexpression such as `a & b & &a & a`.
This PR fixes the issue by setting precedence of reduction ops to lowest.

Example:
```mlir
hw.module @foo(%a: i4, %b: i1) -> (o1:i1) {
  %one4 = hw.constant -1 : i4

  %and1 = comb.icmp eq %a, %one4 : i4
  %and2 = comb.icmp eq %a, %one4 : i4
  %and3 = comb.icmp eq %a, %one4 : i4
  %and = comb.and %and1, %b, %and2, %and3  : i1
  hw.output %and : i1
}
```

`circt-opt -export-verilog` produces:

```verilog
module Foo(
  input  [3:0] a,
  input        b,
  output       o1);

  assign o1 = (&a) & b & (&a) & (&a);
endmodule
```

* Update CI OS version and docker image

* Use v3

* Update buildAndTest.yml

.github/workflows/buildAndTest.yml

@@ -14,16 +14,20 @@ jobs:
     name: Sanity Check
     runs-on: ubuntu-latest
     container:
-      image: ghcr.io/circt/images/circt-ci-build:20220216182244
+      image: ghcr.io/circt/images/circt-ci-build:20240213211952
     steps:
       # Clone the CIRCT repo and its submodules. Do shallow clone to save clone
       # time.
       - name: Get CIRCT
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
         with:
           fetch-depth: 2
           submodules: "false"
 
+      - name: Set git safe
+        run: |
+          git config --global --add safe.directory $PWD
+
       # --------
       # Lint the CIRCT C++ code.
       # -------
@@ -93,7 +97,7 @@ jobs:
       # Upload the format patches to an artifact (zip'd) associated
       # with the workflow run. Only run this on a failure.
       - name: Upload format patches
-        uses: actions/upload-artifact@v2
+        uses: actions/upload-artifact@v3
         continue-on-error: true
         if: ${{ failure() }}
         with:
@@ -119,12 +123,12 @@ jobs:
   # Configure CIRCT using LLVM's build system ("Unified" build). We do not actually build this configuration since it isn't as easy to cache LLVM artifacts in this mode.
   configure-circt-unified:
     name: Configure Unified Build
-    runs-on: ubuntu-18.04
+    runs-on: ubuntu-22.04
     steps:
       # Clone the CIRCT repo and its submodules. Do shallow clone to save clone
       # time.
       - name: Get CIRCT
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
         with:
           fetch-depth: 2
           submodules: "true"
@@ -150,7 +154,7 @@ jobs:
     needs: sanity-check
     runs-on: ubuntu-latest
     container:
-      image: ghcr.io/circt/images/circt-ci-build:20220216182244
+      image: ghcr.io/circt/images/circt-ci-build:20240213211952
     strategy:
       matrix:
         compiler:
@@ -175,7 +179,7 @@ jobs:
       # Clone the CIRCT repo and its submodules. Do shallow clone to save clone
       # time.
       - name: Get CIRCT
-        uses: actions/checkout@v2
+        uses: actions/checkout@v3
         with:
           fetch-depth: 2
           submodules: "true"
@@ -196,7 +200,7 @@ jobs:
       # Try to fetch LLVM from the cache.
       - name: Cache LLVM
         id: cache-llvm
-        uses: actions/cache@v2
+        uses: actions/cache@v3
         with:
           path: |
             llvm/build
@@ -292,7 +296,7 @@ jobs:
       # Upload the tidy patches to an artifact (zip'd) associated
       # with the workflow run. Only run this on a failure.
       - name: Upload tidy patches
-        uses: actions/upload-artifact@v2
+        uses: actions/upload-artifact@v3
         continue-on-error: true
         if: ${{ failure() }}
         with:

lib/Conversion/ExportVerilog/ExportVerilog.cpp

@@ -1755,22 +1755,6 @@ SubExprInfo ExprEmitter::emitBinary(Operation *op, VerilogPrecedence prec,
   if (!isa<AddOp, MulOp, AndOp, OrOp, XorOp>(op))
     rhsPrec = VerilogPrecedence(prec - 1);
 
-  // Introduce extra parentheses to specific patterns of expressions.
-  // If op is "AndOp", and rhs is Reduction And, the output is like `a & &b`.
-  // This is syntactically valid but some tool produces LINT warnings. Also it
-  // would be confusing for users to read such expressions.
-  bool emitRhsParentheses = false;
-  if (auto rhsICmp = op->getOperand(1).getDefiningOp<ICmpOp>()) {
-    if ((rhsICmp.isEqualAllOnes() && isa<AndOp>(op)) ||
-        (rhsICmp.isNotEqualZero() && isa<OrOp>(op))) {
-      if (isExpressionEmittedInline(rhsICmp)) {
-        os << '(';
-        emitRhsParentheses = true;
-        rhsPrec = LowestPrecedence;
-      }
-    }
-  }
-
   // If the RHS operand has self-determined width and always treated as
   // unsigned, inform emitSubExpr of this.  This is true for the shift amount in
   // a shift operation.
@@ -1782,8 +1766,6 @@ SubExprInfo ExprEmitter::emitBinary(Operation *op, VerilogPrecedence prec,
 
   auto rhsInfo = emitSubExpr(op->getOperand(1), rhsPrec, operandSignReq,
                              rhsIsUnsignedValueWithSelfDeterminedWidth);
-  if (emitRhsParentheses)
-    os << ')';
 
   // SystemVerilog 11.8.1 says that the result of a binary expression is signed
   // only if both operands are signed.
@@ -1804,7 +1786,11 @@ SubExprInfo ExprEmitter::emitUnary(Operation *op, const char *syntax,
                                    bool resultAlwaysUnsigned) {
   os << syntax;
   auto signedness = emitSubExpr(op->getOperand(0), Selection).signedness;
-  return {Unary, resultAlwaysUnsigned ? IsUnsigned : signedness};
+  // For reduction operators "&" and "|", make precedence lowest to avoid
+  // emitting an expression like `a & &b`, which is syntactically valid but some
+  // tools produce LINT warnings.
+  return {isa<ICmpOp>(op) ? LowestPrecedence : Unary,
+          resultAlwaysUnsigned ? IsUnsigned : signedness};
 }
 
 /// This function split the output buffer into multiple lines if the emitted

test/Conversion/ExportVerilog/hw-dialect.mlir

@@ -992,13 +992,17 @@ hw.module @addParenthesesToSuccessiveOperators(%a: i4, %b: i1, %c: i4) -> (o1:i1
   %zero4 = hw.constant 0 : i4
   // CHECK: wire [[GEN:.+]] = &c;
 
-  %0 = comb.icmp eq %a, %one4 : i4
-  %and = comb.and %b, %0 : i1
-  // CHECK-NEXT: assign o1 = b & (&a);
-
-  %1 = comb.icmp ne %a, %zero4 : i4
-  %or = comb.or %b, %1 : i1
-  // CHECK-NEXT: assign o2 = b | (|a);
+  %and1 = comb.icmp eq %a, %one4 : i4
+  %and2 = comb.icmp eq %a, %one4 : i4
+  %and3 = comb.icmp eq %a, %one4 : i4
+  %and = comb.and %and1, %b, %and2, %and3  : i1
+  // CHECK-NEXT: assign o1 = (&a) & b & (&a) & (&a);
+
+  %or1 = comb.icmp ne %a, %zero4 : i4
+  %or2 = comb.icmp ne %a, %zero4 : i4
+  %or3 = comb.icmp ne %a, %zero4 : i4
+  %or = comb.or %or1, %b, %or2, %or3  : i1
+  // CHECK-NEXT: assign o2 = (|a) | b | (|a) | (|a);
 
   %3 = comb.icmp eq %c, %one4 : i4
   %multiuse = comb.and %3, %3 : i1