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.

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

[CI] Enable assertions for CI #987

Merged
merged 4 commits into from
Dec 17, 2022

Conversation

LyricZhao
Copy link
Contributor

@LyricZhao LyricZhao commented Dec 16, 2022

Enable assertions by a new CMake build type TRITON_REL_BUILD_WITH_ASSERTS.

@LyricZhao LyricZhao requested a review from ptillet as a code owner December 16, 2022 06:58
@ptillet
Copy link
Collaborator

ptillet commented Dec 16, 2022

I don't think we should switch to debug mode in the CI. This will slow down compilation too much and make it untractable. is RelWithDebInfo somehow insufficient?

@LyricZhao LyricZhao changed the title [CI] Change CMake compilation mode into DEBUG [WIP][CI] Change CMake compilation mode into DEBUG Dec 16, 2022
@LyricZhao
Copy link
Contributor Author

@ptillet @Superjomn
Hi, the problem is due to the commit 9f27468, at line https://github.com/openai/triton/blob/9f27468377b3dbf1b702c26f5abcfb557b1818e0/lib/Conversion/TritonToTritonGPU/TritonToTritonGPU.cpp#L226.

The commit passed CI but however it can not compile on my own machine, with an error (also seen as current CI results):

/home/phil/.triton/llvm/llvm+mlir-14.0.0-x86_64-linux-gnu-ubuntu-18.04-assert/include/llvm/Support/Casting.h:58:23: error: ‘classof’ is not a member of ‘mlir::TritonGPUTypeConverter’
       58 |     return To::classof(&Val);
          |            ~~~~~~~~~~~^~~~~~

This is because llvm::cast is implemented as:

template <typename To, typename From>
 [[nodiscard]] inline decltype(auto) cast(From *Val) {
   assert(isa<To>(Val) && "cast<Ty>() argument of incompatible type!");
   return CastInfo<To, From *>::doCast(Val);
 }

, while isa requires TritonGPUTypeConverter to implement classof.

But the problem is that the commit can pass CI because RelWithDebInfo disables all asserts (not only this one). This PR is meant to open all asserts for CI to ensure correctness for every commit.

@LyricZhao
Copy link
Contributor Author

Another thing worth to mention is that the ASSERT_ENABLED_LLVM only have asserts in compiled library, while cast is in a header file that can be ignore by a RelWithDebInfo compiler.

I suggest that we at least align the assertion behaviors.

@ptillet
Copy link
Collaborator

ptillet commented Dec 16, 2022

it seems like the solution would be to define #NDEBUG rather than switch to full-blown Debug mode. Would this work for you?

@LyricZhao
Copy link
Contributor Author

it seems like the solution would be to define #NDEBUG rather than switch to full-blown Debug mode. Would this work for you?

LGTM, I'll change to that.

@LyricZhao LyricZhao changed the title [WIP][CI] Change CMake compilation mode into DEBUG [WIP][CI] Enable assertions for CI Dec 16, 2022
@LyricZhao LyricZhao changed the title [WIP][CI] Enable assertions for CI [CI] Enable assertions for CI Dec 16, 2022
CMakeLists.txt Outdated Show resolved Hide resolved
CMakeLists.txt Outdated Show resolved Hide resolved
@@ -223,7 +223,7 @@ struct TritonDotPattern : public OpConversionPattern<triton::DotOp> {
ConversionPatternRewriter &rewriter) const override {
RankedTensorType origType = op.getType().cast<RankedTensorType>();
auto origShape = origType.getShape();
auto typeConverter = cast<TritonGPUTypeConverter>(getTypeConverter());
auto typeConverter = static_cast<TritonGPUTypeConverter*>(getTypeConverter());
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this bypass the classof check? Seems not safe.

Copy link
Collaborator

@ptillet ptillet Dec 16, 2022

Choose a reason for hiding this comment

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

Same comment here. Isn't cast safer than static_cast here?

Copy link
Contributor Author

@LyricZhao LyricZhao Dec 17, 2022

Choose a reason for hiding this comment

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

@ptillet
Yes, I agree. I've changed to auto typeConverter = getTypeConverter<TritonGPUTypeConverter>(), which leverages the MLIR interface and looks better.

The official implement of this interface is:

   template <typename ConverterTy>
   std::enable_if_t<std::is_base_of<TypeConverter, ConverterTy>::value,
                    ConverterTy *>
   getTypeConverter() const {
     return static_cast<ConverterTy *>(typeConverter);
   }

, which is also static_cast but safely checks using std::is_base_of.

@Superjomn Superjomn changed the title [CI] Enable assertions for CI [WIP][CI] Enable assertions for CI Dec 16, 2022
@LyricZhao LyricZhao changed the title [WIP][CI] Enable assertions for CI [CI] Enable assertions for CI Dec 16, 2022
@Superjomn Superjomn enabled auto-merge (squash) December 17, 2022 01:04
@Superjomn Superjomn merged commit 707553c into triton-lang:triton-mlir Dec 17, 2022
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