This is an archive of the discontinued LLVM Phabricator instance.

[ADT] Remove SFINAE constraint from llvm::iterator_range ctor for gcc-7
ClosedPublic

Authored by steakhal on Jul 17 2023, 4:28 AM.

Details

Summary

It turns out the SFINAE constraint breaks building MLIR using GCC-7,
which is an outdated, but supported compiler by llvm-project.

I tried to find a solution for fixing it, but I decided to cut branches
and just simply remove the SFINAE constraint until we drop GCC-7.
It was originally introduced by D152891.

Allegedly, GCC-8 and above builds just fine.

Depends on D152891

Fixes https://github.com/llvm/llvm-project/issues/63843

Diff Detail

Event Timeline

steakhal created this revision.Jul 17 2023, 4:28 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 17 2023, 4:28 AM
steakhal requested review of this revision.Jul 17 2023, 4:28 AM
steakhal edited the summary of this revision. (Show Details)Jul 17 2023, 4:30 AM
steakhal updated this revision to Diff 541180.Jul 17 2023, 12:17 PM

Guard the SFINAE by #if as suggested in https://github.com/llvm/llvm-project/issues/63843#issuecomment-1638162336

FYI the end curly is now suddenly clang-formatted to a new line, IDK why.

steakhal retitled this revision from [ADT] Remove SFINAE constraint from llvm::iterator_range ctor to [ADT] Remove SFINAE constraint from llvm::iterator_range ctor for gcc-7.Jul 17 2023, 12:18 PM
mehdi_amini accepted this revision.Jul 17 2023, 12:54 PM

LG: in general I don't like ifdef for compiler version when not strictly needed, but this seems not very intrusive and this makes it easier to clean-up the next time we upgrade the minimum GCC version.

This revision is now accepted and ready to land.Jul 17 2023, 12:54 PM
MaskRay accepted this revision.Jul 17 2023, 2:26 PM
MaskRay added a subscriber: MaskRay.

Allegedly, GCC-8 and above builds just fine.

It would be nice to test this:)

llvm/include/llvm/ADT/iterator_range.h
47

If you know what things are specifically broken, better to use something like:

Work around a bug that ...

Allegedly, GCC-8 and above builds just fine.

It would be nice to test this:)

I'll check that tomorrow.

llvm/include/llvm/ADT/iterator_range.h
47

The thing is that usually I don't write generic code. So I gave up after 4 hours, thus I don't really know why it doesn't work for gcc-8.
So I cannot be more specific.
I had a quick look at gcc-8 release notes, but I couldn't find anything related to templates.

MaskRay added inline comments.Jul 17 2023, 4:56 PM
llvm/include/llvm/ADT/iterator_range.h
47

immolo from Gentoo reports a build success with GCC 8, so only GCC 7 needs the workaround.

... and reported a typo (no => not): "Be careful not to break gcc-7 on the mlir target."

crobeck added a subscriber: crobeck.EditedJul 17 2023, 5:39 PM

Allegedly, GCC-8 and above builds just fine.

It would be nice to test this:)

I'll check that tomorrow.

FWIW, the lit checks were failing for me with with GCC 8.3 so that GCC version check may need to be more inclusive.

immolo added a subscriber: immolo.Jul 17 2023, 7:02 PM

Allegedly, GCC-8 and above builds just fine.

It would be nice to test this:)

I'll check that tomorrow.

FWIW, the lit checks were failing for me with with GCC 8.3 so that GCC version check may need to be more inclusive.

For some added information I used gcc-8.5 for this test.

Can we land this quickly? I'd like to restore the gcc7 build and I'll change my bot to target it.

Allegedly, GCC-8 and above builds just fine.

It would be nice to test this:)

I'll check that tomorrow.

FWIW, the lit checks were failing for me with with GCC 8.3 so that GCC version check may need to be more inclusive.

For some added information I used gcc-8.5 for this test.

I tested it this morning.
gcc-8 (Ubuntu 8.4.0-3ubuntu2) 8.4.0
gcc-7 (Ubuntu 7.5.0-6ubuntu2) 7.5.0
All these builds this just fine.

Can we land this quickly? I'd like to restore the gcc7 build and I'll change my bot to target it.

Pushed. Thank you all for the reviews!

llvm/include/llvm/ADT/iterator_range.h
47

AH, I just noticed now. If you don't mind, I won't fix that typo.

FWIW, the lit checks were failing for me with with GCC 8.3 so that GCC version check may need to be more inclusive.

Don't start this 😅
At this point, I'm very much out of my comfort zone.
I had to install custom PPA only to get gcc-7 and gcc-8 on my system, as those are no longer available officially.
I managed to get 7.5.0 and 8.4.0, so I could fix this. However, to get 8.3, I'd assume I need to compile gcc only for this.
And I'm not sure if we should expect all minor versions of a compiler. To me, it makes more sense to support only the latest revision of a major compiler version.

I intentionally, waited for the Harbormaster to see breakages. Also, after I landed the patch, I waited a couple of days until I pushed some other changes to build on top of this feature to make sure we didn't need to revert many changes if something breaks.
IMO if we would really care about these compilers, we should have them in BuildKite.

FWIW, the lit checks were failing for me with with GCC 8.3 so that GCC version check may need to be more inclusive.

Don't start this 😅
At this point, I'm very much out of my comfort zone.
I had to install custom PPA only to get gcc-7 and gcc-8 on my system, as those are no longer available officially.
I managed to get 7.5.0 and 8.4.0, so I could fix this. However, to get 8.3, I'd assume I need to compile gcc only for this.
And I'm not sure if we should expect all minor versions of a compiler. To me, it makes more sense to support only the latest revision of a major compiler version.

I intentionally, waited for the Harbormaster to see breakages. Also, after I landed the patch, I waited a couple of days until I pushed some other changes to build on top of this feature to make sure we didn't need to revert many changes if something breaks.
IMO if we would really care about these compilers, we should have them in BuildKite.

Oh, sure. Sorry, what I was trying, unsuccessfully apparently, to get across was that it built fine but then failed during the lit tests. Not about the actual GCC version.

steakhal added a comment.EditedJul 18 2023, 4:20 AM

FWIW, the lit checks were failing for me with with GCC 8.3 so that GCC version check may need to be more inclusive.

Oh, sure. Sorry, what I was trying, unsuccessfully apparently, to get across was that it built fine but then failed during the lit tests. Not about the actual GCC version.

I see. Could you please provide me some details?

  • What is you cmake configuration?
  • What is the ninja target to trigger the failure?

To be fair, I'm not exactly sure how could an excluded SFINAE break some tests at runtime :|
Please check if the baseline already has that test failure.

crobeck added a comment.EditedJul 18 2023, 5:43 AM

cmake -GNinja -DLLVM_ENABLE_PROJECTS='llvm;clang;lld' -DLLVM_TARGETS_TO_BUILD='X86;AMDGPU' \
-DCMAKE_BUILD_TYPE=Release -DLLVM_ENABLE_ASSERTIONS=ON

is the build line I was using. The unit that crashes is:

/llvm-project/llvm/unittests/Support/IndexedAccessorTest.cpp

gcc 8.3.1 just happened to be the default on my system (RHEL 8.2). Although looking at it again, ninja doesn't actually build that file until you start the lit tests so it's not actually a runtime error. If things are passing on the build bot can probably just call it "done" and move on.

I'm leaving for vacation until next week, thus I wont be able to fix any new concerns until the llvm release branches off.
I'll most likely keep reading Phabricator but won't produce patches. Feel free to resolve any upcoming issues or revert anything necessary around this patch.