This is an archive of the discontinued LLVM Phabricator instance.

[mlir][python] Remove "Raw" OpView classes
ClosedPublic

Authored by rkayaith on Feb 11 2023, 3:07 PM.

Details

Summary

The raw OpView classes are used to bypass the constructors of OpView
subclasses, but having a separate class can create some confusing
behaviour, e.g.:

op = MyOp(...)
# fails, lhs is 'MyOp', rhs is '_MyOp'
assert type(op) == type(op.operation.opview)

Instead we can use __new__ to achieve the same thing without a
separate class:

my_op = MyOp.__new__(MyOp)
OpView.__init__(my_op, op)

Diff Detail

Event Timeline

rkayaith created this revision.Feb 11 2023, 3:07 PM
Herald added a project: Restricted Project. · View Herald Transcript
rkayaith added inline comments.Feb 11 2023, 6:38 PM
mlir/lib/Bindings/Python/IRCore.cpp
1643–1645

Not sure what the original concern with __new__ was, it seems valid to use as far as I can tell. I didn't see any discussion about this on the original review either: https://reviews.llvm.org/D89932

rkayaith published this revision for review.Feb 11 2023, 6:40 PM
rkayaith added a reviewer: stellaraccident.
rkayaith edited the summary of this revision. (Show Details)Feb 11 2023, 6:42 PM

friendly ping

stellaraccident accepted this revision.Feb 26 2023, 10:30 PM

Thanks. I had started reviewing this but got side tracked with the archaeology. Thanks for simplifying -- this was always not great.

mlir/lib/Bindings/Python/IRCore.cpp
1643–1645

I don't remember either but I do remember struggling with it. I'm +1 on simplifying since this seems to work.

This revision is now accepted and ready to land.Feb 26 2023, 10:30 PM
This revision was automatically updated to reflect the committed changes.