Skip to content

Add bicg testbench, Sub and FNeg conversion - #178

Merged
n0thingNoob merged 9 commits into
mainfrom
testbench
Oct 27, 2025
Merged

n0thingNoob merged 9 commits into
mainfrom
testbench

Conversation

@n0thingNoob

Copy link
Copy Markdown
Collaborator

@YanzhouTang Can you try this bicg testbench? I have merged the latest commit you submitted, but the generated IR still contains the Constant Op inside.

- Merged main branch changes including LlvmSelectToNeuraSel pattern
- Preserved local modifications: LlvmMemsetToNeuraOps, LlvmFNegToNeuraFSub, LlvmSubToNeuraSub patterns
- Resolved merge conflict in LlvmToNeuraPass.cpp

Copilot AI left a comment

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.

Pull Request Overview

This PR adds support for the bicg (BiConjugate Gradient) benchmark testbench and implements conversions for LLVM Sub and FNeg operations to corresponding Neura dialect operations. The changes also include a memset intrinsic handler, though the PR description focuses on addressing remaining Constant Op issues in the generated IR.

Key Changes

  • Added comprehensive bicg kernel testbench with RUN commands for compilation, lowering, mapping, and verification
  • Implemented LLVM FNeg to Neura FSub conversion (negation via subtraction from zero)
  • Implemented LLVM Sub to Neura Sub conversion for integer subtraction

Reviewed Changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 5 comments.

File Description
test/e2e/bicg/bicg_kernel.mlir New end-to-end testbench for bicg kernel with compilation pipeline and FileCheck assertions
lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Added pattern rewrites for memset intrinsic, FNeg, and Sub operations

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread test/e2e/bicg/bicg_kernel.mlir
@YanzhouTang

Copy link
Copy Markdown
Collaborator

@YanzhouTang Can you try this bicg testbench? I have merged the latest commit you submitted, but the generated IR still contains the Constant Op inside.

Thanks, I'll review this PR today 😄

@YanzhouTang

Copy link
Copy Markdown
Collaborator

Hi @n0thingNoob I've checked the bicg case, After folding constant, it does have some operators not folded:

Take operator %0 for example

    %0 = "neura.constant"() <{value = "%arg0"}> : () -> i32
    %1 = "neura.constant"() <{value = "%arg1"}> : () -> i32
    %2 = "neura.constant"() <{value = 0 : i64}> : () -> i64
    %3 = "neura.icmp"(%0) <{cmpType = "sgt"}> {rhs_value = 0 : i32} : (i32) -> i1

This situation is expected because I set a safe line that operation must have at least 1 operator not be folded. So even %0 can be folded, it won't.

So here comes a question @tancheng

  1. If the 2 operators are all constant numbers:
    %0 = "neura.constant"() <{value = 0 : i64}> : () -> i32
    %1 = "neura.constant"() <{value = 1 : i64}> : () -> i32
    %2 = "neura.icmp"(%0, %1) <{cmpType = "sgt"}> : (i32, i32) -> i1

It is reasonable to fold all the constant:

%0 = "neura.icmp"() <{cmpType = "sgt", lhs_value = 0 : i32, rhs_value = 1 : i32}> : () -> i1

And it is natural to calculate %0 as a constant directly:

%0 = "neura.constant"() <{value = false : i1}> : () -> i1
  1. But if there is a function argument in operators:
    %0 = "neura.constant"() <{value = "%arg0"}> : () -> i32
    %1 = "neura.constant"() <{value = 1 : i64}> : () -> i32
    %2 = "neura.icmp"(%0, %1) <{cmpType = "sgt"}> : (i32, i32) -> i1

After folding all constant:

%0 = "neura.icmp"() <{cmpType = "sgt", lhs_value = "%arg1" : i32, rhs_value = 0 : i32}> : () -> i1

We cannot calculate the value of %0 😭 and it will case error when running subsequent PASS --canonicalize-live-in, --leverage-predicated-value,and --transform-ctrl-to-data-flow

So how shall we deal with this problem 🤔

@tancheng

tancheng commented Oct 26, 2025 •

Copy link
Copy Markdown
Contributor
  • We can leverage some existing pass before lowering to neura to fuse operation with two constant operands (two non-arg constants), at least C++ -O3 will do this. i.e., compiler will do the calculation at static compilation time.
    • After fusing, it is not %0 = "neura.icmp"() <{cmpType = "sgt", lhs_value = 0 : i32, rhs_value = 1 : i32}> : () -> i1, instead, it should be a calculated value being folded into the following consumer operation.
  • Argument is treated as constant in our context as we anyways need to preload them into a constant queue in HW. But semantically, argument is not constant. So "We cannot calculate the value of %0" yes.
  • Sorry, I forgot to mention our current RTL design only support one constant operand folding for a given operation. So if there is an op requires 3 operands, and 2 of them are constant, we can only fold 1, not 2. @YanzhouTang can you please add/update this constraint in your code?

Anyways, @n0thingNoob it is okay to have constant op left. I was wrong if I said there shouldn't any const op left in the IR. I will update the RTL to enable this. But I believe the FIR test @Jackcuii is working on has no constant op.

@YanzhouTang

Copy link
Copy Markdown
Collaborator
  • We can leverage some existing pass before lowering to neura to fuse operation with two constant operands (two non-arg constants), at least C++ -O3 will do this. i.e., compiler will do the calculation at static compilation time.

    • After fusing, it is not %0 = "neura.icmp"() <{cmpType = "sgt", lhs_value = 0 : i32, rhs_value = 1 : i32}> : () -> i1, instead, it should be a calculated value being folded into the following consumer operation.
  • Argument is treated as constant in our context as we anyways need to preload them into a constant queue in HW. But semantically, argument is not constant. So "We cannot calculate the value of %0" yes.

  • Sorry, I forgot to mention our current RTL design only support one constant operand folding for a given operation. So if there is an op requires 3 operands, and 2 of them are constant, we can only fold 1, not 2. @YanzhouTang can you please add/update this constraint in your code?

Anyways, @n0thingNoob it is okay to have constant op left. I was wrong if I said there shouldn't any const op left in the IR. I will update the RTL to enable this. But I believe the FIR test @Jackcuii is working on has no constant op.

Got it. I'll add the constraint 🚀

@n0thingNoob

Copy link
Copy Markdown
Collaborator Author
  • We can leverage some existing pass before lowering to neura to fuse operation with two constant operands (two non-arg constants), at least C++ -O3 will do this. i.e., compiler will do the calculation at static compilation time.

    • After fusing, it is not %0 = "neura.icmp"() <{cmpType = "sgt", lhs_value = 0 : i32, rhs_value = 1 : i32}> : () -> i1, instead, it should be a calculated value being folded into the following consumer operation.
  • Argument is treated as constant in our context as we anyways need to preload them into a constant queue in HW. But semantically, argument is not constant. So "We cannot calculate the value of %0" yes.

  • Sorry, I forgot to mention our current RTL design only support one constant operand folding for a given operation. So if there is an op requires 3 operands, and 2 of them are constant, we can only fold 1, not 2. @YanzhouTang can you please add/update this constraint in your code?

Anyways, @n0thingNoob it is okay to have constant op left. I was wrong if I said there shouldn't any const op left in the IR. I will update the RTL to enable this. But I believe the FIR test @Jackcuii is working on has no constant op.

I see. And is there anything I need to change for this PR?

Comment thread test/e2e/bicg/bicg_kernel.mlir
Comment thread test/e2e/bicg/bicg_kernel.mlir Outdated
Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread lib/Conversion/LlvmToNeura/LlvmToNeuraPass.cpp Outdated
Comment thread test/e2e/bicg/bicg_kernel.mlir Outdated
@n0thingNoob
n0thingNoob merged commit 1e7194d into main Oct 27, 2025
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.

4 participants