[comb] New Canonicalization ~sext(x) = sext(~x)

[cowardsa]

New canonicalization pattern pushing logical negation over sign-extension:

~sext(x) = sext(~x)

This represents a canonicalization pattern in that it allows for optimisations as used in synthesis flows that need to identify sign-extended operators to simplify the logic.
Appreciate that it could be debatable whether this is a canonicalization so open to suggestions or discussion?

An example is included in the datapath tests where moving the negation allows for a reduction in the logic required to implement arithmetic operators.

[seldridge]

Generally, having this canonicalizer makes sense to me. It should be cheaper to invert fewer bits. Sign extension (static extract + replication) is viewed as basically free whereas inversion occurs a gate and power cost.

[seldridge]

Can this be written using an ODS pattern as opposed to open coded here?

(This is a usual ask as it's viewed that the ODS patterns are easier to maintain. I do not see existing comb canonicalizations, though. Examples from FIRRTL DIalect: https://github.com/llvm/circt/blob/main/include/circt/Dialect/FIRRTL/FIRRTLCanonicalization.td)

[uenoku]

I'm not sure it's trivial to write ODS pattern for this pattern at Comb mainly because there is no sign type/implicit width extension op like FIRRTL. The ODS would looks like this:

let n = width of x in
let m = width of sign-extended expression in
(xor (concat (replicate (extract(x, n - 1 , 1)), m - n + 1), extract(x, 0, n -2)), -1)
=> 
let flip_x = xor(x, -1) in
(concat (replicate (extract(flip_x, n -1 , 1)), m - n + 1), extract(flip_x, 0, n -2))

IMO it's a bit more maintainable in C++ with a proper helper function :) If we could define and use SExt pattern as ODS's dag object in source pattern without adding a new operation, it would be easier but not sure it's that's possible today 🤔

[seldridge]

Thanks for the context and sketching it out. Yeah, this looks a bit tricky in ODS.

[uenoku]

Suggested change

	if (matchPattern(op.getOperand(0), m_Sext(m_Any(&base)))) {
	Value base;
	if (matchPattern(op.getResult(), m_Complement(m_Sext(m_Any(&base))))) {

Nesting m_Complement would work.

[cowardsa]

@uenoku - didn't really understand how this works?

As written m_Complement does not compose with itself or m_Sext - as you get an error in converting Value to Operation?

Think I would need to refactor the m_Complement matcher to allow for this composition (maybe a separate PR?_ - for example this current style matches existing Xor Folds:

  // xor(xor(x,1),1) -> x
  // but not self loop
  if (isBinaryNot()) {
    Value subExpr;
    if (matchPattern(getOperand(0), m_Complement(m_Any(&subExpr))) &&
        subExpr != getResult())

[uenoku]

Yeah it look we need to use matchOperandOrValueAtIndex instead of getOperand in m_Complement. I can follow up in another PR so it looks good as is. Thank you for trying it!

return xorOp && xorOp.isBinaryNot() && 
            mlir::detail::matchOperandOrValueAtIndex(op, 0, lhs);