[FIRRTL][OM] Use signed integer attributes

[rwy7]

• In the FIRRTL IntegerConstantOp, use a verifier to ensure that the underlying value (encoded as an attribute) has a signed type.
• Similarly, in the OM IntegerAttr, use an attribute verifier to ensure the underlying value is signed, too.
• Fix the OM shl/shr folders so they return OM IntegerAttrs with signed integers, and
• Fix the python API so that, when converting from a python integer to an OM integer attribute, the OM IntegerAttr is signed.

[seldridge]

This looks great.

Are there tests that can be included to check that the verifiers are locked in?

Edit: also, there's the om.py integration test failure which I didn't investigate.

[seldridge]

Consider:

Suggested change

	if (apInt.isNegative()) {
	++width;
	apInt = apInt.zext(width);
	}
	if (apInt.isNegative())
	apInt = apInt.zext(++width);

[seldridge]

This looks great. Thanks for taking the time to do this correctly and get the width correct.

[seldridge]

Doing this with the verifier is fine. Did you explore changing this from APSIntAttr to some other, stricter constraint? E.g., like how OMIntegerAttr does it? WDYT?

[rwy7]

I didn't for this PR, but something along those lines would be great. Honestly I think we should build out a whole new bigint attribute, rather than using APInts and IntegerAttrs directly.

[uenoku]

circt/lib/Bindings/Python/OMModule.cpp

Line 298 in f5a6e91

return omIntegerAttrGet(intAttr);

Not actually tested but I feel this line is problematic. I think it's ok to consider python integer to si64 (historically we are using i64 for python integer, not ideal thouigh...

[rwy7]

I believe there is a bug here, which I am not fixing in this PR. This folder should be extending the width before shifting. Probably, the evaluator has the same issue.

Demo

firrtl.circuit "Casts" {
  firrtl.module @Casts() {}
  firrtl.class @PropertyArithmetic(out %o1: !firrtl.integer) {
    %0 = firrtl.integer 4
    %1 = firrtl.integer.shl %0, %0 : (!firrtl.integer, !firrtl.integer) -> !firrtl.integer
    firrtl.propassign %o1, %1 : !firrtl.integer
  }
}

// ./bin/circt-opt -canonicalize='top-down=true region-simplify=aggressive' ../scratch/scratch.mlir  --mlir-print-ir-before-all
module {
  firrtl.circuit "Casts" {
    firrtl.module @Casts() {
    }
    firrtl.class @PropertyArithmetic(out %o1: !firrtl.integer) {
      %0 = firrtl.integer 0
      firrtl.propassign %o1, %0 : !firrtl.integer
    }
  }
}

[rwy7]

#9602

[seldridge]

Thanks for filing this issue!

[seldridge]

LGTM

Are there error tests that can be added?

[seldridge]

Nit: this could invert the check and do an early return for less indentation.

[seldridge]

Thanks for filing this issue!