Skip to content

[Triton-MLIR] Fix a few minor issues#689

Merged
ptillet merged 4 commits intotriton-lang:triton-mlirfrom
shintaro-iwasaki:siwasaki/pr/minor_fixes
Sep 22, 2022
Merged

[Triton-MLIR] Fix a few minor issues#689
ptillet merged 4 commits intotriton-lang:triton-mlirfrom
shintaro-iwasaki:siwasaki/pr/minor_fixes

Conversation

@shintaro-iwasaki
Copy link
Contributor

@shintaro-iwasaki shintaro-iwasaki commented Sep 21, 2022

  1. In Triton-MLIR's combine pass, I didn't check if getDefiningOp() returns nullptr or not. Unlike dynamic_cast, llvm::dyn_cast fails if nullptr is passed. The first two commits fix this issue and add tests to check the behavior.

  2. Pipeline transformation in TritonGPU-MLIR uses a wrong index to access the mask argument of insert_slice_async. The last commit fixes this issue.

(Note that the these issues are found to try to compile the tutorial matmul code.)

@shintaro-iwasaki shintaro-iwasaki changed the title Siwasaki/pr/minor fixes [Triton-MLIR] Fix a few minor issues Sep 21, 2022
@shintaro-iwasaki
Copy link
Contributor Author

This PR also updates the version of clang-format on ubuntu-latest based on an update in #662.

With this change, it passes tests on ubuntu-latest: shintaro-iwasaki#6

@Jokeren
Copy link
Contributor

Jokeren commented Sep 21, 2022

Unlike dynamic_cast, llvm::dyn_cast fails if nullptr is passed

I don't quite get this one. Doesn't the following code also fail when nullptr is passed?

    auto *constantMaskCandidate = mask.getDefiningOp();
    if (!constantMaskCandidate)
      return mlir::failure();

@shintaro-iwasaki
Copy link
Contributor Author

shintaro-iwasaki commented Sep 21, 2022

Thanks for taking a look at this PR @Jokeren! Sorry for confusion. The first commit handles the last "NG" case: llvm::dyn_cast does not seem to accept nullptr.

// Assume constantMaskCandidate is nullptr
auto *constantMaskCandidate = mask.getDefiningOp();

// OK
if (!constantMaskCandidate)
  return mlir::failure();

// OK: dynamic_cast<> can take nullptr 
#if 0
// The following code does not work because LLVM does not enable RTTI.
if (!dynamic_cast<arith::ConstantOp *>(constantMaskCandidate))
  return mlir::failure();
#endif

// NG: llvm::dyn_cast<> cannot take nullptr (it causes assert() or SEGV) for some reasons
if (!llvm::dyn_cast<arith::ConstantOp>(constantMaskCandidate))
  return mlir::failure();    

@ptillet
Copy link
Collaborator

ptillet commented Sep 22, 2022

What about llvm::dyn_cast_or_null ?

@shintaro-iwasaki
Copy link
Contributor Author

Thanks, @ptillet. You are completely right. I updated the first commit to use dyn_cast_or_null.

@ptillet ptillet enabled auto-merge (squash) September 22, 2022 03:19
@ptillet ptillet merged commit 940ef3f into triton-lang:triton-mlir Sep 22, 2022
ZzEeKkAa pushed a commit to ZzEeKkAa/triton that referenced this pull request Aug 5, 2024
…ng#689)

Each lane owns a whole TF32 value's bits. By doing so, we can do the
arithmetic operation on the operand A value.

This is aligned to the requirements we have to the OpenCL interface.
brunomazzottiamd pushed a commit to brunomazzottiamd/triton that referenced this pull request Jan 29, 2025
* add perfci tuning shapes and fall back configs

* make test_correctness work with multiple kernels

* change persistent_gemm kernel name back

* add persistent_gemm unit tests

* adapt tune_streamk to be able to switch kernel tunning from command line

* fix output file name issue and merge final TFLOPS and time into yaml file

* remove comments
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.

3 participants