Skip to content
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

Merged
merged 1 commit into from
Aug 3, 2024

Conversation

elhewaty
Copy link
Member

@elhewaty elhewaty commented Aug 3, 2024

No description provided.

Copy link
Member

@maerhart maerhart left a 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;
Copy link
Member

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.

Copy link
Member Author

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
Copy link
Member

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?

Copy link
Member Author

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.

Comment on lines 511 to 515
%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
}
Copy link
Member

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?

Copy link
Member Author

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.

Copy link
Member Author

@elhewaty elhewaty Aug 3, 2024

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
}

@elhewaty elhewaty force-pushed the fix-keepOneOp-canonicalize-crash branch from 0ebf0d0 to e0f22e4 Compare August 3, 2024 17:20
Copy link
Contributor

@fabianschuiki fabianschuiki left a 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

Copy link
Member

@maerhart maerhart left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@elhewaty elhewaty merged commit 17545bc into llvm:main Aug 3, 2024
4 checks passed
@elhewaty elhewaty deleted the fix-keepOneOp-canonicalize-crash branch August 3, 2024 18:07
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants