[FIRRTL] Canonicalize multibit_mux with narrow index

[uenoku]

This adds a canonicalization to optimize multibit_mux whose index has narrow width. Close #7361

[seldridge]

LGTM

While, this looks right to me, it is worthwhile to verify (locally test) that the behavior is doing to right thing for edge cases, e.g., zero width.

[seldridge]

Is 1ull portable across windows and Linux?

[uenoku]

I think it's fine as there are several existing use cases in LLVM and CIRCT repo.

[dtzSiFive]

long long is at least 64bits since c++11, according to https://en.cppreference.com/w/cpp/language/types .

That said (only looking at the code added here):

indexWidth is signed (int32_t), and in particular can be negative (!). Shift by a negative amount is UB! Please add a check for that, as even without UB it's logically not what we want either.

[dtzSiFive]

Great work identifying this and putting together the canonicalizer!

On my "TODO" is to bolster our knownbits-like capabilities, and with that hat on I can't help but wonder about generalizing this beyond index being narrow directly. Future work, but since you're pushing on this just thought I'd mention it 👍 .

Anyway, generally looks good but there's an important issue that needs to be addressed before landing this, see my comment. Thanks!

[dtzSiFive]

long long is at least 64bits since c++11, according to https://en.cppreference.com/w/cpp/language/types .

That said (only looking at the code added here):

indexWidth is signed (int32_t), and in particular can be negative (!). Shift by a negative amount is UB! Please add a check for that, as even without UB it's logically not what we want either.

[uenoku]

On my "TODO" is to bolster our knownbits-like capabilities, and with that hat on I can't help but wonder about generalizing this beyond index being narrow directly. Future work, but since you're pushing on this just thought I'd mention it 👍 .

Yeah that sounds great idea. I think naively computing knownbits could be expensive (as shown in

circt/lib/Dialect/Comb/CombFolds.cpp

Lines 618 to 627 in e8033f1

	#if 0
	// If the extracted bits are all known, then return the result.
	auto knownBits = computeKnownBits(op.getInput())
	.extractBits(cast<IntegerType>(op.getType()).getWidth(),
	op.getLowBit());
	if (knownBits.isConstant()) {
	replaceOpWithNewOpAndCopyName<hw::ConstantOp>(rewriter, op,
	knownBits.getConstant());
	return success();
	}

) so I guess a dedicated pass would be needed. Though computing known bits is exactly bit-sensitive IMCP so it's worth extending the lattice in IMCP.

[dtzSiFive]

Friendly upgrade to "request changes" since this is reachable UB. I don't have the details paged in but the expected change (dropping widths, I think?) to one of the tests was all it took 👍 .

Just making sure this gets addressed, thanks!

[uenoku]

Thanks! I added a condition and test to ensure that the width is not negative number. I checked the canonicalization on internal cores and it was ok.

[dtzSiFive]

LGTM! Thanks!

(sorry for delay, missed the update 👍 )

[uenoku]

Np, thank you for review!