Conversation
There was a problem hiding this comment.
Pull request overview
This PR updates the ReLU kernel test to use a C++ integer-based implementation (relu_int.cpp) instead of the previous C implementation (relu.c). The change simplifies the kernel and updates all test expectations accordingly.
Changes:
- Updated compilation from C (clang, C11) to C++ (clang++, C++17)
- Replaced source file from
relu.ctorelu_int.cpp - Updated test expectations (MAPPING, YAML, ASM checks) to match the simpler integer-based ReLU kernel output
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // | ||
| // Check the mapped MLIR contains proper structure and neura operations. | ||
| // Check the mapped MLIR contains key operations with full statements. | ||
| // RUN: FileCheck %s --input-file=%t-mapping.mlir -check-prefix=MAPPING |
There was a problem hiding this comment.
The FileCheck command for the MAPPING prefix is duplicated on line 29 and line 34. Line 34 appears to be redundant as line 29 already checks the same file with the same prefix. Consider removing the duplicate on line 34, or if both are intentional, add a comment explaining why the same check needs to run twice.
| // RUN: clang -S -emit-llvm -O3 -fno-vectorize -fno-unroll-loops -std=c11 \ | ||
| // RUN: -I %S/../../benchmark/CGRA-Bench/kernels/relu -DSMALL_DATASET \ | ||
| // RUN: -o %t-kernel-full.ll %S/../../benchmark/CGRA-Bench/kernels/relu/relu.c | ||
| // Compile the int ReLU C++ kernel to LLVM IR. |
There was a problem hiding this comment.
The comment says "int ReLU" but doesn't specify what makes it different from the previous version. Consider adding more context about what "int ReLU" means (e.g., "integer-based ReLU" or "ReLU operating on integer arrays") to improve clarity for future readers.
| // Compile the int ReLU C++ kernel to LLVM IR. | |
| // Compile the integer-based ReLU C++ kernel (operating on integer arrays) to LLVM IR. |
tancheng
left a comment
There was a problem hiding this comment.
Shouldn't we upload relu_int.cpp into testbench submodule and sync it in this PR?
Yes, I am doing that right now. |
64e4268 to
d751c36
Compare
Using the relu.cpp in for_loop to replace the original relu kernel