-
Notifications
You must be signed in to change notification settings - Fork 385
[Synth ] feat : Fold Majority Graph #9033
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
base: main
Are you sure you want to change the base?
Conversation
aed800d to
d5dcd5b
Compare
lib/Dialect/Synth/SynthOps.cpp
Outdated
| // If i and j are constant. | ||
| if (auto c1 = getConstant(i)) { | ||
| if (auto c2 = getConstant(j)) { | ||
| // If both constants are equal, we can fold. | ||
| if (*c1 == *c2) { | ||
| // auto value = cast<IntegerAttr>(getInputs()[i]).getValue(); | ||
| // return IntegerAttr::get(IntegerType::get(getContext(), | ||
| // value.getBitWidth()),value); | ||
| return IntegerAttr::get(getType(), *c1); | ||
| } | ||
| // If constants are complementary, we can fold. | ||
| if (*c1 == ~*c2) | ||
| return getOperand(k); | ||
| } | ||
| } |
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.
This block would be dead as two constants are already handled.
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.
yes handled !
lib/Dialect/Synth/SynthOps.cpp
Outdated
| auto width = inputs[0].getBitWidth(); | ||
| APInt result(width, 0); | ||
|
|
||
| // is every input of same width ? |
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.
yes
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.
okay!
lib/Dialect/Synth/SynthOps.cpp
Outdated
| // x 1 1 | ||
| if (inputValues.size() == 2) { | ||
| if (inputValues[0] != inputValues[1]) | ||
| return {}; |
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 think it's pessimistic. If two constants are same you can return constants, if they are complement you can return third variable.
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.
true , resolved !
lib/Dialect/Synth/SynthOps.cpp
Outdated
| if (isInverted(i) != isInverted(j)) { | ||
| return getOperand(k); | ||
| } | ||
| return getOperand(i); |
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.
This is a my bug but it's necessary to check if operand i is inverted. Could you fix it as well?
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.
Hi @uenoku ,
if operand(i) is inverted , should i return {} ?
I can't return with inverted operand(i) , let canonicalise handle it !
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!
| } | ||
| } | ||
| } | ||
| } |
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.
you need to return {} here as evaluate expects same operands. Also evaluate expects uninverted values so the current code is not correct.
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.
yes , it is handled now
Thank you for pointing out
7c95602 to
272d8e2
Compare
|
Hi @uenoku , Can you please check if this resolves the folding issue ? |
| assert(inputs.size() == getNumOperands() && | ||
| "Number of inputs must match number of operands"); | ||
|
|
||
| if (inputs.size() == 3) { |
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.
Could you keep the previous implementation? evaluate is meant to evaluate the op based on input values so it's necessary to look at inversion.
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.
Sorry for delayed review!
lib/Dialect/Synth/SynthOps.cpp
Outdated
| IntegerType::get(getContext(), inputValues[0].getBitWidth()), | ||
| inputValues[0]); | ||
| else | ||
| return getOperand(index); // fix if it is a Inverted |
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.
Could you check the inversion?
| // Pattern match following cases: | ||
| // maj_inv(x, x, y) -> x | ||
| // maj_inv(x, y, not y) -> x |
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.
Could you keep the canonicalization since folder is essentially a bit weaker?
lib/Dialect/Synth/SynthOps.cpp
Outdated
| SmallVector<APInt, 3> inputValues; | ||
| size_t i = 0, index; |
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.
Could you do slightly change the code structure to store non-constant index and check based on the number constants?
| SmallVector<APInt, 3> inputValues; | |
| size_t i = 0, index; | |
| SmallVector<APInt, 3> inputValues; | |
| SmallVector<unsigned, 3> nonConstantIndex; | |
| for (auto [i, input] : llvm::enumerate(adaptor.getInputs())) { | |
| auto attr = llvm::dyn_cast_or_null<IntegerAttr>(input); | |
| if (attr) | |
| inputValues.push_back(attr); | |
| else | |
| nonConstantIndex.push_back(i); | |
| } | |
| if (inputValues.size() == getNumINputs()) | |
| return IntegerAttr::get(getType(), evaluate(inputValues)); | |
| if (getNumOperands() != 3) | |
| return {}; | |
| if (nonConstantIndex.size() == 1) { | |
| // Get two constants. | |
| auto idx = nonConstantIndex.front(); | |
| auto constIndex1 = (idx+1)%3; | |
| auto constIndex2 = (idx+2)%3; | |
| auto c1 = adaptor.getInputs()[constIndex1]; | |
| auto c2 = ... | |
| auto ...; | |
| } | |
|
Hi @uenoku , Can you suggest how can i handle ~x in fold any way i can return ~x as i can't negate a operand |
|
Really thank you for working hard on this 🙇🙇!
Unfortunately we cannot handle it since it's not allowed to create a new operation: https://mlir.llvm.org/docs/Canonicalization/#canonicalizing-with-the-fold-method |
|
Hi @uenoku , can i handle in fold ? I have commented in the code ,as canoicalisation ishandling it |
74a6f8e to
ff77ae4
Compare
|
The first two can be implemented in a straightforward way. For the 3rd one, i realized maybe MLIR folder allows in-place mutation for the op itself, so maybe you can do something like |
Resolves Partial #8918
// Folder for
maj_inv(x, !x, y) -> y
maj_inv(x, x, y) -> x
maj_inv(x, 0, 1) -> x