Skip to content

Fix merge error and compile error and bugs in buildInitValue #13

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.

Sign up for GitHub

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

Merged
ienkovich merged 1 commit into from
May 30, 2024
Merged

Fix merge error and compile error and bugs in buildInitValue #13

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 2 additions & 1 deletion third_party/cpu/include/TritonToTritonCPU/Passes.td
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -83,7 +83,8 @@ def ConvertHistogramOp : Pass<"triton-cpu-convert-histogram-op", "mlir::ModuleOp

let dependentDialects = ["mlir::arith::ArithDialect",
"mlir::memref::MemRefDialect",
"mlir::vector::VectorDialect",
"mlir::vector::VectorDialect"];
}

def ConvertReductionOp : Pass<"triton-cpu-convert-reduction", "mlir::ModuleOp"> {
let summary = "Convert Triton ReduceOp.";
Expand Down
4 changes: 2 additions & 2 deletions third_party/cpu/lib/TritonToTritonCPU/ConvertReductionOp.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -149,11 +149,11 @@ struct ReduceOpConversion : public OpConversionPattern<triton::ReduceOp> {
else if (kind == vector::CombiningKind::MAXSI)
initVal = rewriter.getIntegerAttr(
elemTy,
static_cast<int64_t>(1UL << (elemTy.getIntOrFloatBitWidth() - 1)));
static_cast<int64_t>(-(1UL << (elemTy.getIntOrFloatBitWidth() - 1))));
Copy link
Copy Markdown
Collaborator Author

@minjang minjang May 30, 2024

Choose a reason for hiding this comment

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

This would be a sort of INT_MIN. But super interestingly, the dump of initVal remained the same? But what do you think? I think this explicit - is clearer for signed int64_t?

Copy link
Copy Markdown
Collaborator

@ienkovich ienkovich May 30, 2024

Choose a reason for hiding this comment

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

Is the point here to always have negative int64_t passed to the attribute? The dump is similar because with your change you modify only those bits that don't matter for the used type.

else if (kind == vector::CombiningKind::MINSI)
initVal = rewriter.getIntegerAttr(
elemTy, static_cast<int64_t>(
1UL << (elemTy.getIntOrFloatBitWidth() - 1) - 1));
(1UL << (elemTy.getIntOrFloatBitWidth() - 1)) - 1));
Copy link
Copy Markdown
Collaborator Author

@minjang minjang May 30, 2024

Choose a reason for hiding this comment

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

Clang compiler complains this:

Operator '<<' has lower precedence than '-'; '-' will be evaluated first (fix available)clang(-Wshift-op-parentheses)

Copy link
Copy Markdown
Collaborator

@ienkovich ienkovich May 30, 2024

Choose a reason for hiding this comment

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

Thanks for the fix. Do you think we also need to change 1UL to 1ULL or use an explicit cast to uint64_t to have all compilers covered?

else if (kind == vector::CombiningKind::MINIMUMF ||
kind == vector::CombiningKind::MINNUMF) {
if (elemTy.isF32())
Expand Down