Skip to content

Enhance the logic of phi -> phi_start transform#295

Merged
tancheng merged 3 commits intocoredac:mainfrom
ShangkunLi:phi_start_debug
Apr 7, 2026
Merged

Enhance the logic of phi -> phi_start transform#295
tancheng merged 3 commits intocoredac:mainfrom
ShangkunLi:phi_start_debug

Conversation

@ShangkunLi
Copy link
Copy Markdown
Collaborator

In this pr,

  • We enhance the logic of transforming phi to phi_start. In the previous logic, we transform all phi with a reserve op as an operand into phi_start. This may violate the semantics in the simulator, so we only transform phi ops with a reserve op + a grant_once op into phi_start ops
  • We modify the codegen pass for the corresponding changes

@ShangkunLi
Copy link
Copy Markdown
Collaborator Author

Hi~ @n0thingNoob,

Since you can handle the phi that has two operands with the false predicate, do we still need phi_start now?

@ShangkunLi
Copy link
Copy Markdown
Collaborator Author

Hi~ @n0thingNoob,

Since you can handle the phi that has two operands with the false predicate, do we still need phi_start now?

After discussion with @n0thingNoob, we can handle this later after evaluation of their project.

@tancheng
Copy link
Copy Markdown
Contributor

  • so we only transform phi ops with a reserve op + a grant_once op into phi_start ops

What we avoid transforming using this PR?

@ShangkunLi
Copy link
Copy Markdown
Collaborator Author

  • so we only transform phi ops with a reserve op + a grant_once op into phi_start ops

What we avoid transforming using this PR?

For ir like:

    %48 = neura.grant_predicate %39, %43 : !neura.data<i64, i1>, !neura.data<i1, i1> -> !neura.data<i64, i1>
    %49 = neura.grant_predicate %39, %43 : !neura.data<i64, i1>, !neura.data<i1, i1> -> !neura.data<i64, i1>
    %50 = neura.grant_predicate %40, %43 : !neura.data<i32, i1>, !neura.data<i1, i1> -> !neura.data<i32, i1>
    %51 = neura.reserve : !neura.data<i32, i1>
    %52 = neura.phi_start %50, %51 : !neura.data<i32, i1>, !neura.data<i32, i1> -> !neura.data<i32, i1>
    %53 = neura.reserve : !neura.data<i64, i1>
    %54 = neura.phi_start %49, %53 : !neura.data<i64, i1>, !neura.data<i64, i1> -> !neura.data<i64, i1>
    %55 = neura.reserve : !neura.data<i64, i1>
    %56 = neura.phi_start %48, %55 : !neura.data<i64, i1>, !neura.data<i64, i1> -> !neura.data<i64, i1>
    %57 = "neura.gep"(%56) <{operandSegmentSizes = array<i32: 0, 1>}> {lhs_value = "%arg3"} : (!neura.data<i64, i1>) -> !neura.data<!llvm.ptr, i1>
    %58 = "neura.load"(%57) : (!neura.data<!llvm.ptr, i1>) -> !neura.data<i32, i1>
    %59 = "neura.icmp"(%58) <{cmpType = "slt"}> {rhs_value = 0 : i32} : (!neura.data<i32, i1>) -> !neura.data<i1, i1>
    %60 = neura.grant_predicate %57, %59 : !neura.data<!llvm.ptr, i1>, !neura.data<i1, i1> -> !neura.data<!llvm.ptr, i1>
    %61 = neura.grant_predicate %54, %59 : !neura.data<i64, i1>, !neura.data<i1, i1> -> !neura.data<i64, i1>
    %62 = neura.grant_predicate %52, %59 : !neura.data<i32, i1>, !neura.data<i1, i1> -> !neura.data<i32, i1>
    %63 = "neura.not"(%59) : (!neura.data<i1, i1>) -> !neura.data<i1, i1>
    %64 = neura.grant_predicate %54, %63 : !neura.data<i64, i1>, !neura.data<i1, i1> -> !neura.data<i64, i1>
    %65 = neura.grant_predicate %52, %63 : !neura.data<i32, i1>, !neura.data<i1, i1> -> !neura.data<i32, i1>

These phi_start don't have any operand with a true predicate when they are executed for the first time. So they actually have the same semantics as phi. I think we should clean this up.

@tancheng
Copy link
Copy Markdown
Contributor

Okay, so it sounds like, if the LHS is neura.grant_predicate, we won't transform it to phi_start; if LHS is neura.grant_once, we do the transformation.

Above summary is correct?

@ShangkunLi
Copy link
Copy Markdown
Collaborator Author

Okay, so it sounds like, if the LHS is neura.grant_predicate, we won't transform it to phi_start; if LHS is neura.grant_once, we do the transformation.

Above summary is correct?

Yes, this is what this pr done.

But after carefully thinking, I think we don't even need a phi_start. Because if we encounter:

  1. phi (<val, 1>, <N/A, N/A>) or phi (<N/A, N/A>, <val, 1>): we should simply choose the one with predicate 1
  2. phi (<N/A>, <N/A>): we should output <AnyVal, 0>

The semantics of phi is robust enough, I think.

@tancheng
Copy link
Copy Markdown
Contributor

Okay, so it sounds like, if the LHS is neura.grant_predicate, we won't transform it to phi_start; if LHS is neura.grant_once, we do the transformation.
Above summary is correct?

Yes, this is what this pr done.

But after carefully thinking, I think we don't even need a phi_start. Because if we encounter:

  1. phi (<val, 1>, <N/A, N/A>) or phi (<N/A, N/A>, <val, 1>): we should simply choose the one with predicate 1
  2. phi (<N/A>, <N/A>): we should output <AnyVal, 0>

The semantics of phi is robust enough, I think.

Well, let's keep phi_start for now. We need phi_start in RTL to enable context-switch across kernels.

@tancheng
Copy link
Copy Markdown
Contributor

tancheng commented Apr 6, 2026

@ShangkunLi do we still want to merge this PR?

@ShangkunLi
Copy link
Copy Markdown
Collaborator Author

@ShangkunLi do we still want to merge this PR?

If we choose to keep phi_start, we don't need to check in this pr.

But the problem is that we may have some semantic overlap between phi_start and phi.

@tancheng
Copy link
Copy Markdown
Contributor

tancheng commented Apr 7, 2026

@ShangkunLi do we still want to merge this PR?

If we choose to keep phi_start, we don't need to check in this pr.

But the problem is that we may have some semantic overlap between phi_start and phi.

We need phi_start to indicate the starting point of the kernel, to serve context-switching, so I prefer to checking in this PR.

@ShangkunLi
Copy link
Copy Markdown
Collaborator Author

@ShangkunLi do we still want to merge this PR?

If we choose to keep phi_start, we don't need to check in this pr.
But the problem is that we may have some semantic overlap between phi_start and phi.

We need phi_start to indicate the starting point of the kernel, to serve context-switching, so I prefer to checking in this PR.

Synced with the main branch now, we can check in this pr first.

@tancheng tancheng merged commit 17c69f0 into coredac:main Apr 7, 2026
1 check passed
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