Use clang-12 to fix issue with LLVM_ABI_BREAKING_CHECKS

[karupayun]

This commit is fixing the root cause of the issue that appeared during an llvm update, and was raised in #4212 (comment).

The issue was temporary fixed in #4512, where we set LLVM_ABI_BREAKING_CHECKS to FORCE_OFF constant in order to not use a non-deterministic seed inside the hashing function (included in this llvm commit: llvm/llvm-project@ce80c80).

A further investigation (with chsigg@) found that the issue is that LLVM is built with clang11, while Triton use a newer version. The ABI issue is brought up here: llvm/llvm-project#96282 (comment), but the consensus seemed to be that this setup is rare.

Updated the clang version to 12 in the ubuntu build fixed the issue and therefore we can revert setting LLVM_ABI_BREAKING_CHECKS constant. I am additionaly erasing LLVM_ABI_BREAKING_CHECKS in the setup of the other hardwares (it seems it was not needed) and I am splitting ubuntu and macOS configurations because it seems cleaner than having a variable that sets the compiler version for each of them.

[ThomasRaoux]

Do we need to update LLVM hash in the same PR?
Why does the macOS build doesn't need the same change as linux one?

[karupayun]

Do we need to update LLVM hash in the same PR?

It's actually not needed since not updating the llvm-hash will mean an old (in the cache) LLVM built with clang-11 but also LLVM_ABI_BREAKING_CHECKS=FORCE_OFF, but I needed to change it in order to test, so I included for completeness.

Why does the macOS build doesn't need the same change as linux one?

I'm actually not sure, but I guess that it's because mac-12 should be using a newer version of clang by default. I could not find which one they are using.

[ThomasRaoux]

does that mean any Triton build with clang <= 11 or gcc would show the error?

[chsigg]

Do we need to update LLVM hash in the same PR?

It sounds risky to update the build flags while keeping the hash, because the prebuilt LLVM stored in Azure would no longer match the build script.

does that mean any Triton build with clang <= 11 or gcc would show the error?

Any build of Triton with clang > 11 or gcc shows the error when linking against LLVM built with clang <= 11 (which is the case before this PR).

[karupayun]

Updated the llvm-hash (after #4791). Everything still green.

Waiting for your input @ThomasRaoux :)

[ThomasRaoux]

sorry for the delay, I was OOO.

does that mean any Triton build with clang <= 11 or gcc would show the error?

Any build of Triton with clang > 11 or gcc shows the error when linking against LLVM built with clang <= 11 (which is the case before this PR).

I'm still trying to understand that part. So right LLVM built with clang <= 11 is broken unless we set DLLVM_ABI_BREAKING_CHECKS=FORCE_OFF in the LLVM build. If move to build LLVM with clang > 11 it will be solved for triton build with clang > 11 but then it would still mismatch when LLVM is built with clang > 11 and triton is built with clang <= 11 right?

I thought the problem was that this preprocessor mismatches between LLVM build and triton?

#if LLVM_ENABLE_ABI_BREAKING_CHECKS &&                                         \
    (!defined(__clang__) || __clang_major__ > 11)

Did I get that wrong?

[chsigg]

sorry for the delay, I was OOO.

does that mean any Triton build with clang <= 11 or gcc would show the error?

Any build of Triton with clang > 11 or gcc shows the error when linking against LLVM built with clang <= 11 (which is the case before this PR).

I'm still trying to understand that part. So right LLVM built with clang <= 11 is broken unless we set DLLVM_ABI_BREAKING_CHECKS=FORCE_OFF in the LLVM build. If move to build LLVM with clang > 11 it will be solved for triton build with clang > 11 but then it would still mismatch when LLVM is built with clang > 11 and triton is built with clang <= 11 right?

That's correct. You can't pass hash containers across a component A built with clang <= 11 and a different component B built with clang > 11 or gcc, unless one uses DLLVM_ABI_BREAKING_CHECKS=FORCE_OFF. Currently A=llvm and B=Triton, but it could also be the other way around.

I thought the problem was that this preprocessor mismatches between LLVM build and triton?

#if LLVM_ENABLE_ABI_BREAKING_CHECKS &&                                         \
    (!defined(__clang__) || __clang_major__ > 11)

Did I get that wrong?

No, you didn't get that wrong. This is the problem.

[ThomasRaoux]

sorry for the delay, I was OOO.

does that mean any Triton build with clang <= 11 or gcc would show the error?

Any build of Triton with clang > 11 or gcc shows the error when linking against LLVM built with clang <= 11 (which is the case before this PR).

I'm still trying to understand that part. So right LLVM built with clang <= 11 is broken unless we set DLLVM_ABI_BREAKING_CHECKS=FORCE_OFF in the LLVM build. If move to build LLVM with clang > 11 it will be solved for triton build with clang > 11 but then it would still mismatch when LLVM is built with clang > 11 and triton is built with clang <= 11 right?

That's correct. You can't pass hash containers across a component A built with clang <= 11 and a different component B built with clang > 11 or gcc, unless one uses DLLVM_ABI_BREAKING_CHECKS=FORCE_OFF. Currently A=llvm and B=Triton, but it could also be the other way around.

I thought the problem was that this preprocessor mismatches between LLVM build and triton?

#if LLVM_ENABLE_ABI_BREAKING_CHECKS &&                                         \
    (!defined(__clang__) || __clang_major__ > 11)

Did I get that wrong?

No, you didn't get that wrong. This is the problem.

Thanks for clarifying. So based on that if we were to keep DLLVM_ABI_BREAKING_CHECKS=FORCE_OFF in the LLVM build it should work with any combination of gcc and llvm while if we remove it it would only work when building both LLVM and triton with LLVM > 11? Isn't it better to keep DLLVM_ABI_BREAKING_CHECKS=FORCE_OFF in this case? Is the motivation mainly clean up the build?

[chsigg]

The motivation is that we want to have checks. NDEBUG builds automatically disable LLVM_ABI_BREAKING_CHECKS, but we currently build with assertions, so ABI breaking checks should be enabled as well.

[ThomasRaoux]

The motivation is that we want to have checks. NDEBUG builds automatically disable LLVM_ABI_BREAKING_CHECKS, but we currently build with assertions, so ABI breaking checks should be enabled as well.

I see your point. What kind of problem could that catch?
I'm a bit worried about potentially breaking the workflow of users using gcc or an older clang, especially since the error generated is hard to track to this. If we want to do that we should at least make clang > 11 a requirement.
Let me discuss with others to see if they have a feel of how likely it is to break developers flow out there.

[ThomasRaoux]

actually, would that only break triton when built with clang <= 11? If so maybe we just need to require clang > 11 which would be reasonable

[chsigg]

Yes, only the issue is clang <= 11 on one side vs clang > 11 or gcc on the other. If we don't use clang <= 11 for LLVM or Triton, everything is fine.

Clang 11 is the default for the GitHub Ubuntu 20.04 runner image though, so there is a chance that people will run into this.

I can look into making the build fail explicitly for bad compiler combinations.

[ThomasRaoux]

I can look into making the build fail explicitly for bad compiler combinations.

That would be great. I think we should at least make clang minimum version to 12. It should be easy to do in cmake

[chsigg]

The non-pic workaround in LLVM has been removed (llvm/llvm-project#110952). We can therefore remove the LLVM_ABI_BREAKING_CHECKS=FORCE_OFF again. Anyone building LLVM with clang <= 11 and non-pic will get a compilation error. If this is acceptable for LLVM upstream, it should be acceptable for Triton users as well.

[ThomasRaoux]

The non-pic workaround in LLVM has been removed (llvm/llvm-project#110952). We can therefore remove the LLVM_ABI_BREAKING_CHECKS=FORCE_OFF again. Anyone building LLVM with clang <= 11 and non-pic will get a compilation error. If this is acceptable for LLVM upstream, it should be acceptable for Triton users as well.

We should still make clang 12 a minimum requirement to build triton or is this not needed?

[chsigg]

We should still make clang 12 a minimum requirement to build triton or is this not needed?

No, not needed. I doubt anyone will build Triton with non-pic, and if anyone does and happens to use clang-11 for that it will produce a compile error. Specifically, the risk of silent ODR-violations no longer exists.

[ThomasRaoux]

That looks good thanks for explaining the problem in detail. I'm probably doing something wrong but I can't find the hash 5255b819c1b117e6048f354c035f5d36f3afad2b in llvm?

[ThomasRaoux]

weird I can't find this hash in llvm repo?

[chsigg]

Yeah, sorry. I grabbed the wrong commit. The right one is def9550c9b389eaa3adbac3089acf06a61a6fdb5. I will just revert the llvm-hash change and then this should be mergable.