Skip to content

add support of neura.PhiOp in interpreter#37

Closed
Yfeng-44 wants to merge 1 commit intocoredac:mainfrom
Yfeng-44:interpret_phi
Closed

add support of neura.PhiOp in interpreter#37
Yfeng-44 wants to merge 1 commit intocoredac:mainfrom
Yfeng-44:interpret_phi

Conversation

@Yfeng-44
Copy link
Copy Markdown
Collaborator

Add neura.PhiOp interpreter support

to verify this patch ./build/tools/neura-interpreter/neura-interpreter /home/yuan/neura_dataflow/dataflow/test/neura/interpreter/phiOp_interpret.mlir

@Yfeng-44 Yfeng-44 requested a review from tancheng June 15, 2025 19:46
@Yfeng-44
Copy link
Copy Markdown
Collaborator Author

#28

auto incoming = valueMap[operand];
if (incoming.predicate) {
result = incoming;
break; // Found the active value.
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Move the comment above the if.

@@ -0,0 +1,20 @@
// RUN: neura-interpreter %s | FileCheck %s

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Can we run mlir-neura-opt with --assign-accelerator --leverage-predicated-value so the IR would contain PredicatedValue as https://github.com/coredac/dataflow/blob/64e7b3537cc5de007ec81b706c67cb20f0ce2860/test/neura/ctrl/branch_no_arg.mlir#L62
and then run neura-interpreter again to double check?

} else if (auto phiOp = dyn_cast<neura::PhiOp>(op)) {
PredicatedData result{0.0f, false}; // Default to a false predicate.
// Find the one operand with a true predicate.
for (Value operand : phiOp.getOperands()) {
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Shouldn't we assert there must be at most one operand with predicate as true?

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Plz hold on, let me re-think the design. Its current form cannot support the diagram shown in README: https://github.com/tancheng/CGRA-Mapper

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I introduced grant_predicate and grant_once in #39, so we don't need to complicate phi's definition for now, so you are good to go: "assert there must be at most one operand with predicate as true".

Later on, we can introduce new op to fuse the (phi and grant_once and const) if needed.

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Rename the test as phi_interpret.mlir.

@tancheng
Copy link
Copy Markdown
Contributor

@tancheng tancheng closed this Sep 19, 2025
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.

2 participants