-
Notifications
You must be signed in to change notification settings - Fork 299
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[Arc] Make the canonicalizer shuffle the input vector elements before merging #7394
[Arc] Make the canonicalizer shuffle the input vector elements before merging #7394
Conversation
4c6e9cf
to
c1a1f0d
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Very neat! Cool to see that the shuffling allows you to merge more vectorize ops 😃.
Can you check whether this works on one of the cores in circt/arc-tests
? Once it does I think this is ready to go! 🥳
if (tempVec != SmallVector<Value>(otherVecOp.getResults().begin(), | ||
otherVecOp.getResults().end())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Does tempVec != otherVecOp.getResults()
not work here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I will try, but I think getResults()
returns result_range
which cannot be converted into a SmallVector<Value>
but is worth trying!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There should be an implicit conversion from SmallVector<Value>
to an iterator range, which should theoretically be able to compare against a result range. Worth trying, don't worry if it doesn't work 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't work!
error: invalid operands to binary expression ('SmallVector<mlir::Value>' and '::mlir::Operation::result_range' (aka 'mlir::ResultRange'))
if (tempVec != otherVecOp.getResults())
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for checking 😃
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@fabianschuiki, should I land this now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's 12:17 AM here, so please land it if it's Okay.
The canonicalizer crashes 😢, I will test it before pushing but I need to reduce the test case. |
c1a1f0d
to
7818e2b
Compare
It works on the dual rocket core |
No description provided.