[Comb] Integer Overflow Analysis

[cowardsa]

Building on the Integer Range Analysis for the Comb dialect. We now implement an overflow detection to determine when a particular operation truncates a carry-out bit. For example:

x[3:0] = a[3:0] + b[3:0]; // Carry-out truncated
y[4:0] = a[3:0] + b[3:0]; // Carry-out retained

This analysis will prove valuable for proving the correctness of downstream transformations to follow in a subsequent PR. The analysis inserts the result as an attribute that can be carried forward through a lowering pass to the datapath dialect, which will decompose basic operations, making them difficult to analyze locally.

[uenoku]

Cool! I think this makes sense. I believe conceptually this is same asnuw in llvm. Can we can borrow the terminology (comb.nuw maybe?) for the consistency?

Not blocking but I even think we should add overflow attribute as part of comb.add/mul/sub etc instead of discardable attributes following Arith/llvm dialect:
https://github.com/llvm/llvm-project/blob/eaedab226cfcf99b92fbfc91b502096a11b45de8/mlir/include/mlir/Dialect/Arith/IR/ArithOps.td#L229-L232

Extending Comb ops can be done later so no blocking at all.

[uenoku]

Please use WalkPatternRewriteDriver since this is one-shot.

[uenoku]

Probably assertion is ok (maybe static_assert is better) since CombOpTy is a template type.

Suggested change

	if (!isa<comb::AddOp>(op) && !isa<comb::MulOp>(op) && !isa<comb::SubOp>(op))
	assert(isa<comb::AddOp, comb::MulOp, comb::SubOp>(op));

[cowardsa]

Thanks for the comments @uenoku - have implemented the non-blocking changes.

With regards to making overflow flags part of the operator - I think we need to agree on what the equivalent statement is for comb (from the arith description):
"If the nuw and/or nsw flags are present, and an unsigned/signed overflow occurs (respectively), the result is poison."

Presumably the same is also sort of true - but I guess one would not want your synthesis engine to just optimise away hardware which it thinks is poison?

[uenoku]

LGTM

[uenoku]

Maybe polarity is opposite? Also can we use UnitAttr instead of BoolAttr?

Suggested change

	op->setAttr("comb.nuw", BoolAttr::get(op->getContext(), overflowed));
	if(!overflowed)
	op->setAttr("comb.nuw", UnitAttr::get(op->getContext()));

[cowardsa]

Apologies - yes my mistake - will update the polarity

[uenoku]

"If the nuw and/or nsw flags are present, and an unsigned/signed overflow occurs (respectively), the result is poison."
Presumably the same is also sort of truex

I think this is true.

but I guess one would not want your synthesis engine to just optimise away hardware which it thinks is poison?

I agree we might not exploit poison to optimize aggregissively like LLVM does, but however every transformation that relies on nuw flag will assume that the operation never overflow, so the transformation will be entirely invalid if the operation actually overflows. From that perspective it's actually valid to consider overflow with nuw flag to be poison and replace them with arbitary value.