Skip to content
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

Jump to bottom

Fix build failure in issues #178 #187

Open
wants to merge 6 commits into
base: main
Choose a base branch
from
Open

Fix build failure in issues #178 #187

wants to merge 6 commits into from

Conversation

zhaoshiz
Copy link

Fixing below compilation errors in include/triton-shared/Conversion/TritonArithToLinalg/ConversionPatterns.hpp:

/home/runner/work/triton-shared/triton-shared/triton_shared/include/triton-shared/Conversion/TritonArithToLinalg/ConversionPatterns.hpp:839:68: error: no member named 'getFile' in 'mlir::triton::AssertOp'
llvm::formatv("{0}.py:{1}: {2} Assertion {3} failed", op.getFile(),
~~ ^
/home/runner/work/triton-shared/triton-shared/triton_shared/include/triton-shared/Conversion/TritonArithToLinalg/ConversionPatterns.hpp:840:26: error: no member named 'getLine' in 'mlir::triton::AssertOp'
op.getLine(), op.getFunc(), op.getMessage());
~~ ^
/home/runner/work/triton-shared/triton-shared/triton_shared/include/triton-shared/Conversion/TritonArithToLinalg/ConversionPatterns.hpp:840:40: error: no member named 'getFunc' in 'mlir::triton::AssertOp'
op.getLine(), op.getFunc(), op.getMessage());
~~ ^
3 errors generated.

This fix builds with triton @ab07e5472bcb414a0c8dd7ecab80f84370c4894e, and llvm @cfd3289a1f9a87e220737a634904a886a82d424a.

@zhaoshiz zhaoshiz mentioned this pull request Nov 1, 2024
Open
@nhat-nguyen
Copy link
Collaborator

@zhaoshiz Thank you so much! Would you mind also updating the submodule commit to match in this PR too? Otherwise, our build will fail.

@zhaoshiz
Copy link
Author

zhaoshiz commented Nov 6, 2024

@nhat-nguyen, I've updated the submodule sha and fixed additional compilation errors. I'm working with legal dept. to get the CLA approved.

@zhaoshiz
Copy link
Author

@microsoft-github-policy-service agree company="Qualcomm Innovation Center, Inc."

@mdehling
Copy link

Thank you, this saved me some time! :)

mdehling
mdehling reviewed Nov 20, 2024
View reviewed changes
include/triton-shared/Conversion/TritonArithToLinalg/ConversionPatterns.hpp Outdated
@@ -836,8 +836,7 @@ struct AssertConverter : public OpConversionPattern<triton::AssertOp> {
}

auto assertMessage =
llvm::formatv("{0}.py:{1}: {2} Assertion `{3}` failed", op.getFile(),
op.getLine(), op.getFunc(), op.getMessage());
llvm::formatv("{0}.py:{1}: {2} Assertion `{3}` failed", op.getMessage());
Copy link

Choose a reason for hiding this comment

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

The format string doesn't seem to match the argument. Am I missing something?

Copy link
Collaborator

Choose a reason for hiding this comment

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

ah good catch. @zhaoshiz were there any issues with the other arguments that required you to remove them?

Copy link

Choose a reason for hiding this comment

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

This is the error shown in the first comment. Triton AssertOp's no longer have these functions, only a message.

cf. triton-lang/triton@185ad59

Copy link
Author

@zhaoshiz zhaoshiz Nov 22, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

yea, good catch, I should look more carefully. Yes, Triton has removed File, Line, Func arguments from its AssertOp

Copy link
Collaborator

Choose a reason for hiding this comment

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

looks like we have a few failures. would you mind taking a look at those? you can xfail the ones in Conversion/TritonToLinalg because we're retiring the pass soon. thank you!

Copy link
Author

Choose a reason for hiding this comment

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

looks like we have a few failures. would you mind taking a look at those? you can xfail the ones in Conversion/TritonToLinalg because we're retiring the pass soon. thank you!

yes, I'm looking through those test failures.

Copy link
Author

@zhaoshiz zhaoshiz Dec 2, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

I managed to fix/xfail most of the failures except:
TRITON-SHARED :: Conversion/StructuredToMemref/wraparound_side_by_side.mlir
TRITON-SHARED :: Conversion/StructuredToMemref/wraparound_stacked.mlir

The assertion is caused by LLVM commit llvm/llvm-project@889b67c, which consideres <2x?> and <?x?> are mismatch while both are dynamic shapes. I asked if that's intended:
llvm/llvm-project@889b67c#r149804171

Copy link
Author

Choose a reason for hiding this comment

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

I fixed the failure StructuredToMemref/get_num_programs.mlir: 2153c53 after syncing-up with Triton@acc25d91fba850c18c099e7e577962ba56bdd06c and
LLVM@86b69c31642e98f8357df62c09d118ad1da4e16a.

Copy link
Collaborator

Choose a reason for hiding this comment

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

thank you! looks like we have some failure in the "Prepare CPU backend" step. Doesn't look immediately clear to me what could be the cause. Perhaps the path that triton uses to store llvm has changed in the newer commits?

Copy link
Author

Choose a reason for hiding this comment

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

hmm... I did see a symlink to my llvm being created in ${HOME}/.triton/llvm/, and the two commands below worked fine in my local env:
file $(pwd)/build/$(ls $(pwd)/build | grep -i cmake)/third_party/triton_shared/tools/triton-shared-opt/triton-shared-opt
ls ${HOME}/.triton/llvm/$(ls ${HOME}/.triton/llvm/ | grep -i llvm)/bin

is there a verbose log of gh workflow?

@zhaoshiz zhaoshiz closed this Dec 12, 2024
@zhaoshiz
Fix build failure in issues microsoft#178
6422548 
Fixing below compilation errors in include/triton-shared/Conversion/TritonArithToLinalg/ConversionPatterns.hpp:

  /home/runner/work/triton-shared/triton-shared/triton_shared/include/triton-shared/Conversion/TritonArithToLinalg/ConversionPatterns.hpp:839:68: error: no member named 'getFile' in 'mlir::triton::AssertOp'
          llvm::formatv("{0}.py:{1}: {2} Assertion `{3}` failed", op.getFile(),
                                                                  ~~ ^
  /home/runner/work/triton-shared/triton-shared/triton_shared/include/triton-shared/Conversion/TritonArithToLinalg/ConversionPatterns.hpp:840:26: error: no member named 'getLine' in 'mlir::triton::AssertOp'
                        op.getLine(), op.getFunc(), op.getMessage());
                        ~~ ^
  /home/runner/work/triton-shared/triton-shared/triton_shared/include/triton-shared/Conversion/TritonArithToLinalg/ConversionPatterns.hpp:840:40: error: no member named 'getFunc' in 'mlir::triton::AssertOp'
                        op.getLine(), op.getFunc(), op.getMessage());
                                      ~~ ^
  3 errors generated.

This fix builds with triton @ab07e5472bcb414a0c8dd7ecab80f84370c4894e,
and llvm @cfd3289a1f9a87e220737a634904a886a82d424a.
@zhaoshiz
Fix build failures in microsoft#178 and update Triton
3afe5d2 
Fix compilation errors caused by LLVM commits:
f18c3e4e7335df282c468b6dff3d29be1822a96d [mlir][Transforms] Dialect Conversion: Simplify materialization fn result type (#113031)
8c4bc1e75de27adfbaead34b895b0efbaf17bd02 [mlir][Transforms] Merge 1:1 and 1:N type converters (#113032)

Update Triton to 94684d326723b67b146f23f342623ea058a32098
@zhaoshiz
Fix format string in ConversionPatterns.hpp
b703f1c 
Remove parts of llvm::formatv string correspond to File, Line and Func
arguments of triton::AssertOp.
@zhaoshiz
Add new members to CPUOptions/CPUDriver and fix/xfail test failures
29fd6a6 
Add `sanitize_overflow: bool = True` to class CPUOptions in compiler.py
and `get_benchmarker(self)` to class CPUDriver in driver.py to run the
tests.

XFAILing TritonToLinalg tests since this pass will be retire soon:
test/Conversion/TritonToLinalg/wraparound_side_by_side.mlir
test/Conversion/TritonToLinalg/wraparound_stacked.mlir

XFAILing StructuredToMemref tests due to LLVM commit
889b67c9d30e3024a1317431d66c22599f6c2011 asserts that dynamic shapes
like <2x?> and <?x?> are mismatch:
test/Conversion/StructuredToMemref/wraparound_side_by_side.mlir
test/Conversion/StructuredToMemref/wraparound_stacked.mlir
@zhaoshiz zhaoshiz reopened this Dec 12, 2024
zhaoshiz and others added 2 commits December 11, 2024 17:29
@zhaoshiz
Rebase and sync-up with Triton and LLVM
7823105 
Update CMakeList.txt for python and pybind11 headers.
Fixed test/Conversion/TritonArithToLinalg/split.mlir.
Working on test/Conversion/StructuredToMemref/get_num_programs.mlir.

Builds with Triton@acc25d91fba850c18c099e7e577962ba56bdd06c and
LLVM@86b69c31642e98f8357df62c09d118ad1da4e16a.
Rewrite triton::SplatOp in PtrAnalysis
2153c53 
Add rewriteSplatOp() in PtrAnalysis pass. This function creates a
tts.makeptr for the case below:
    %6 = tt.splat %arg0 : !tt.ptr<i32> -> tensor<1x!tt.ptr<i32>>

Previously we rely on rewriteAddPtrOp to create the tts.makeptr:
    %3 = arith.constant 0 : index
    %6 = tt.splat %arg0 : !tt.ptr<i32> -> tensor<1x!tt.ptr<i32>>
    %7 = tt.addptr %6, %3 : tensor<1x!tt.ptr<i32>>, tensor<1xi32>

Creating a constant 0 and adding it to a pointer is optimized away by
Triton.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mdehling mdehling mdehling left review comments

@nhat-nguyen nhat-nguyen nhat-nguyen left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@zhaoshiz @nhat-nguyen @mdehling