-
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] Fix crash in the arc canonicalizer produced by the KeepOneVecOp optimization #7429
Conversation
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.
Good job finding this! Just some question about whether we couldn't simplify the regression test a little more.
rewriter.replaceAllUsesWith(currentBlock.getArgument(argIdx - shuffledBy), | ||
currentBlock.getArgument(in->second)); | ||
currentBlock.eraseArgument(argIdx - shuffledBy); | ||
++shuffledBy; | ||
changed = true; | ||
continue; | ||
} | ||
inExists[input] = argIdx; | ||
inExists[input] = argIdx - shuffledBy; |
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.
Nit: instead of incrementing argIdx
every iteration and subtracting the number of args we have already deleted, we could only increment argIdx
when we don't delete the argument.
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, that's better. can we handle this in auto [argIdx, inputVec] : llvm::enumerate(vecOp.getInputs())
, should I refactor the loop?
%4 = arc.call @Queue_24_arc(%arg1, %arg1, %arg3, %arg3) : (i1, i1, i1, i1) -> i124 | ||
arc.vectorize.return %4 : i124 | ||
} | ||
%3 = arc.state @dummy(%2#0, %3) clock %clock latency 1 : (i124, i124) -> i124 |
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.
Nit: any particular reason this state
(and the arc it's referring to) has to be here or could we just add %2
as module output?
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.
No particular reason for this. I just wanted to use %2
, I don't know how to use it as a module output, if we don't use it, it will be eliminated by the optimizer.
%1:2 = arc.vectorize (%clock, %clock) : (!seq.clock, !seq.clock) -> (i1, i1) { | ||
^bb0(%arg0: !seq.clock): | ||
%4 = arc.state @TLXbar_8_arc() clock %arg0 latency 1 : () -> i1 | ||
arc.vectorize.return %4 : i1 | ||
} |
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.
Is this arc.vectorize
necessary? Isn't just the other one triggering this issue?
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 isn't, I just needed multiple inputs to handle the shuffles. I will handle it with another input.
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.
how about this? If there's a way to return the result from the module, that would be great as we will get rid of the arc.
arc.define @dummy(%arg0: i1, %arg1: i1) -> i1 {
%0 = comb.or %arg0, %arg1 : i1
arc.output %0 : i1
}
hw.module @Repeat_agian(in %clock : !seq.clock, in %0 : i1, in %in1: i1, in %in2: i1) {
%2:2 = arc.vectorize (%in1, %in2), (%in1, %in2), (%0, %0), (%0, %0) : (i1, i1, i1, i1, i1, i1, i1, i1) -> (i1, i1) {
^bb0(%arg0: i1, %arg1: i1, %arg2: i1, %arg3: i1):
%4 = comb.or %arg0, %arg1, %arg2, %arg3 : i1
arc.vectorize.return %4 : i1
}
%3 = arc.state @dummy(%2#0, %3) clock %clock latency 1 : (i1, i1) -> i1
hw.output
}
0ebf0d0
to
e0f22e4
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 nice find and fix! 🥳 LGTM
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!
No description provided.