[OM] Python bindings for om integer #6042

prithayan · 2023-09-05T14:44:14Z

Implement the python bindings for the om.integer attribute. Also ensure all tests use om::IntegerAttr instead of mlir::IntegerAttr.

lib/Dialect/OM/Evaluator/Evaluator.cpp

mikeurbach

The basic bindings parts look good, just left some questions about a couple parts that I don't really understand the need for.

mikeurbach · 2023-09-08T15:20:58Z

include/circt-c/Dialect/OM.h

+
+MLIR_CAPI_EXPORTED bool omAttrIsAIntegerAttr(MlirAttribute attr);
+
+MLIR_CAPI_EXPORTED MlirAttribute omIntegerAttrGetAttr(MlirAttribute attr);


nit: i think our convention so far is usually to just call it "blahTypeAttrGet", you don't need the final "Attr" in "omIntegerAttrGetAttr".

mikeurbach · 2023-09-08T15:24:58Z

include/circt/Dialect/OM/Evaluator/Evaluator.h

@@ -64,6 +65,21 @@ struct AttributeValue : EvaluatorValue {
  Attribute attr;
 };

+struct IntegerValue : EvaluatorValue {


Do we really need a new kind of EvaluatorValue? It seems like this could just be an AttributeValue. How is IntegerValue different from a normal AttributeValue, maybe I'm missing something, but I don't see it in this PR.

Agreed, I think we don't need this.

mikeurbach · 2023-09-08T15:27:44Z

lib/CAPI/Dialect/OM.cpp

@@ -171,15 +171,23 @@ MlirAttribute omEvaluatorValueGetPrimitive(OMEvaluatorValue evaluatorValue) {

 /// Get the Primitive from an EvaluatorValue, which must contain a Primitive.
 OMEvaluatorValue omEvaluatorValueFromPrimitive(MlirAttribute primitive) {
+  auto attr = unwrap(primitive);
+  // Create om::IntegerAttr for any mlir::IntegerAttr.


I don't think we need to do this conversion. It seems like this sort of logic shouldn't belong in the C API. What was the motivation for this part? We have conversion passes to make sure we're using the right types of data by the time we get to the evaluator.

I think this conversion is required if we want to send integer constants from evaluator. Otherwise there will be type mismatch. The other alternative would be to create om.integer constants in the evaluator script, instead of
obj = evaluator.instantiate("Test", 42)
For example without this explicit conversion evaluator would error out,

Evaluator (INFO) actual parameter: 42 : i64 Evaluator (INFO) format parameter type: '!om.integer'

I see, thanks for clarifying. But, I still think the C API is not the right place for the conversion. I think we should generally avoid any logic in the C API layer besides converting to and from C++ objects. Could we do this in the Python code behind the instantiate API that wraps integers to always wrap them as om.integer attributes?

Thanks Mike, that makes sense, I will try that.

Yes, I think the right place would be to add the similar logic to dialects/om.py:unwrap_python_object.

prithayan · 2023-09-14T13:59:57Z

@uenoku , @mikeurbach , Addressed the review comments, please take another look at this PR.

mikeurbach

This looks good to me, thanks for the iterations on this.

mikeurbach · 2023-09-14T15:37:58Z

lib/Bindings/Python/OMModule.cpp

@@ -308,7 +308,7 @@ PythonValue omEvaluatorValueToPythonValue(OMEvaluatorValue result) {

 OMEvaluatorValue pythonValueToOMEvaluatorValue(PythonValue result) {
  if (auto *attr = std::get_if<MlirAttribute>(&result))
-    return omEvaluatorValueFromPrimitive(*attr);
+    return omEvaluatorValueFromPrimitive(omCastIntAttrIfValid(*attr));


This cast is needed here because we're still using the var_to_attribute from support.py right? I think this current implementation is fine--thanks for moving the cast to this point. Down the road, the OM evaluator might want to have it's own version of var_to_attribute, which always creates OM integer for Python integers.

I'm also wondering if we need something similar for the other primitive types, like strings, booleans, etc. If we do, then I think we should probably just define our own var_to_attribute for OM specifically.

I agree with Mike. omCastIntAttrIfValid might be redundant if we tweaked om.py:unwrap_python_object/wrap_mlir_value.

Cast mlir::IntegerAttr to om::IntegerAttr

mikeurbach

Thanks for the iterations, I think this is looking great.

mikeurbach · 2023-09-19T19:52:20Z

lib/CAPI/Dialect/OM.cpp

+      (Attribute)unwrap(attr).cast<circt::om::IntegerAttr>().getValue());
+}
+
+MlirAttribute omCastIntAttrIfValid(MlirAttribute attr) {


Should this just be something like omIntegerAttrGet(MlirAttribute attr), and do a direct cast<mlir::IntegerAttr>(unwrap(attr))? I think the way you've refactored the Python, we just need a method to get an om::IntegerAttr from an mlir::IntegerAttr.

Thanks @mikeurbach , you are right, simplified the API.

lib/CAPI/Dialect/OM.cpp

mikeurbach · 2023-09-19T19:54:15Z

lib/Bindings/Python/dialects/om.py

@@ -38,11 +38,16 @@ def wrap_mlir_object(value):
  assert isinstance(value, BaseObject)
  return Object(value)

+def om_var_to_attribute(obj, none_on_fail: bool = False) -> ir.Attrbute:
+  if isinstance(obj, int):


I think we'll need similar functionality for strings, bools, etc. right? That can come as follow ups.

I think for bools we are still using i1 , so mlir::BoolAttr should suffice.
Seems like StringAttr` also doesn't require an explicit cast.
Will add lit tests for string and bool, and the API if required.

uenoku

LGTM, thanks!

prithayan marked this pull request as draft September 5, 2023 14:58

uenoku reviewed Sep 5, 2023

View reviewed changes

lib/Dialect/OM/Evaluator/Evaluator.cpp Outdated Show resolved Hide resolved

prithayan force-pushed the dev/pbarua/om_int_evaluator branch 2 times, most recently from 8e4784a to c1ed204 Compare September 6, 2023 16:37

prithayan marked this pull request as ready for review September 6, 2023 17:29

prithayan changed the title ~~[OM] Evaluator accept om.integer attribute~~ [OM] Python bindings for om integer Sep 6, 2023

prithayan requested review from uenoku and mikeurbach September 8, 2023 13:45

mikeurbach reviewed Sep 8, 2023

View reviewed changes

prithayan force-pushed the dev/pbarua/om_int_evaluator branch 2 times, most recently from e5aebd6 to 6186898 Compare September 14, 2023 13:59

mikeurbach approved these changes Sep 14, 2023

View reviewed changes

prithayan added 3 commits September 19, 2023 12:12

[OM] Add om.Integer python bindings

d2e94ee

Cast mlir::IntegerAttr to om::IntegerAttr

Rebase

f270f32

Add integerAttr get API

c7a3ee1

prithayan force-pushed the dev/pbarua/om_int_evaluator branch from 6186898 to c7a3ee1 Compare September 19, 2023 19:46

mikeurbach reviewed Sep 19, 2023

View reviewed changes

Address review comments

f0ce742

prithayan force-pushed the dev/pbarua/om_int_evaluator branch from e7aa58b to f0ce742 Compare September 19, 2023 20:17

prithayan merged commit 10da65b into main Sep 19, 2023
5 checks passed

uenoku reviewed Sep 20, 2023

View reviewed changes

prithayan deleted the dev/pbarua/om_int_evaluator branch April 11, 2024 16:07

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[OM] Python bindings for om integer #6042

[OM] Python bindings for om integer #6042

prithayan commented Sep 5, 2023 •

edited

Loading

mikeurbach left a comment

mikeurbach Sep 8, 2023

mikeurbach Sep 8, 2023

uenoku Sep 11, 2023

mikeurbach Sep 8, 2023

prithayan Sep 8, 2023 •

edited

Loading

mikeurbach Sep 8, 2023

prithayan Sep 8, 2023

uenoku Sep 11, 2023

prithayan commented Sep 14, 2023

mikeurbach left a comment

mikeurbach Sep 14, 2023

mikeurbach Sep 14, 2023

uenoku Sep 18, 2023

mikeurbach left a comment

mikeurbach Sep 19, 2023

prithayan Sep 19, 2023

mikeurbach Sep 19, 2023

prithayan Sep 19, 2023

uenoku left a comment


		MLIR_CAPI_EXPORTED bool omAttrIsAIntegerAttr(MlirAttribute attr);

		MLIR_CAPI_EXPORTED MlirAttribute omIntegerAttrGetAttr(MlirAttribute attr);

[OM] Python bindings for om integer #6042

[OM] Python bindings for om integer #6042

Conversation

prithayan commented Sep 5, 2023 • edited Loading

mikeurbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prithayan Sep 8, 2023 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prithayan commented Sep 14, 2023

mikeurbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mikeurbach left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

uenoku left a comment

Choose a reason for hiding this comment

prithayan commented Sep 5, 2023 •

edited

Loading

prithayan Sep 8, 2023 •

edited

Loading