llvm / llvm/circt

[FIRRTL] incorrect type inference for mux

Open
#8,767 0 comments 1 reaction 0 assignees View on GitHub
bug FIRRTL
Dominant language
C++
Stars
2.2k
Forks
524
Avg merge
3d 2h
Merged PRs (30d)
46

Description

```firrtl
FIRRTL version 4.1.0
circuit Foo:
public module Foo:
input sel1 : UInt<1>
input a : UInt<8>[1]
input b : UInt<8>
output o : UInt<8>
connect o, mux(sel1, a, b)
```
gives:
```mlir
firrtl.circuit "Foo" {
firrtl.module @Foo(in %sel1: !firrtl.uint<1>, in %a: !firrtl.vector, 1>, in %b: !firrtl.uint<8>, out %o: !firrtl.uint<8>) attributes {convention = #firrtl} {
%0 = firrtl.mux(%sel1, %a, %b) : (!firrtl.uint<1>, !firrtl.vector, 1>, !firrtl.uint<8>) -> !firrtl.uint<8>
firrtl.matchingconnect %o, %0 : !firrtl.uint<8>
}
}
```
According to the FIRRTL spec, mux requires that the high and low arguments are equivalent, and so this program should have been rejected. Eventually, this crashes in `LowerToHW`. The code in `inferMuxReturnType` mistakenly assumes that the high signal is an `IntType` without checking:
```c++
// Two different Int types can be compatible. If either has unknown width,
// then return it. If both are known but different width, then return the
// larger one.
if (type_isa(low)) {
int32_t highWidth = high.getBitWidthOrSentinel();
int32_t lowWidth = low.getBitWidthOrSentinel();
if (lowWidth == -1)
return low.getConstType(outerTypeIsConst);
if (highWidth == -1)
return high.getConstType(outerTypeIsConst);
return (lowWidth > highWidth ? low : high).getConstType(outerTypeIsConst);
}
```

Contributor guide

No contributing guide indexed for this repository

Research direction

Start at inferMuxReturnType and inspect how the mux operands are checked before LowerToHW. Use the FIRRTL reproducer in the issue to verify that mismatched vector and integer operands are rejected rather than lowered into an invalid result or causing a crash.

Written by the indexing model from the issue text.

Assessment

Tech stack
cpp
Domain
compilers
Issue type
Bug
Difficulty
3/5
Estimated time
1-2 days
Activity status
Stale
Clarity
Clearly specified
Newbie friendliness
55/100

Get new issues in your inbox

A short digest of beginner-friendly GitHub issues.