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

[steakhal]

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

[steakhal]

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.

[mehdi_amini]

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.

[MaskRay]

Allegedly, GCC-8 and above builds just fine.

It would be nice to test this:)

llvm/include/llvm/ADT/iterator_range.h

37

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

Work around a bug that ...

[steakhal]

In D155441#4507877, @MaskRay wrote:

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

37

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]

llvm/include/llvm/ADT/iterator_range.h

37

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]

In D155441#4507897, @steakhal wrote:

In D155441#4507877, @MaskRay wrote:

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]

In D155441#4508400, @crobeck wrote:

In D155441#4507897, @steakhal wrote:

In D155441#4507877, @MaskRay wrote:

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.

[mehdi_amini]

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

[steakhal]

In D155441#4508583, @immolo wrote:

In D155441#4508400, @crobeck wrote:

In D155441#4507897, @steakhal wrote:

In D155441#4507877, @MaskRay wrote:

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.

In D155441#4509003, @mehdi_amini wrote:

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

37

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

[steakhal]

In D155441#4508400, @crobeck wrote:

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.

[crobeck]

In D155441#4509069, @steakhal wrote:

In D155441#4508400, @crobeck wrote:

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]

In D155441#4509885, @crobeck wrote:

In D155441#4509069, @steakhal wrote:

In D155441#4508400, @crobeck wrote:

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]

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.

[steakhal]

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.

llvm/include/llvm/ADT/iterator_range.h

 #define LLVM_ADT_ITERATOR_RANGE_H
 
 #include "llvm/ADT/ADL.h"
 #include <type_traits>
 #include <utility>
 
 namespace llvm {
 
-template <typename From, typename To, typename = void>
-struct explicitly_convertible : std::false_type {};
-
-template <typename From, typename To>
-struct explicitly_convertible<
-    From, To,
-    std::void_t<decltype(static_cast<To>(
-        std::declval<std::add_rvalue_reference_t<From>>()))>> : std::true_type {
-};
-
 /// A range adaptor for a pair of iterators.
 ///
 /// This just wraps two iterators into a range-compatible interface. Nothing
 /// fancy at all.
 template <typename IteratorT>
 class iterator_range {
   IteratorT begin_iterator, end_iterator;
 
 public:
-  template <typename Container,
-            std::enable_if_t<explicitly_convertible<
-                detail::IterOfRange<Container>, IteratorT>::value> * = nullptr>
+  // TODO: Add SFINAE to test that the Container's iterators match the range's
+  //       iterators. Be careful no to break gcc-7 on the mlir target.
+  //       See https://github.com/llvm/llvm-project/issues/63843
+  template <typename Container>
   iterator_range(Container &&c)
       : begin_iterator(adl_begin(std::forward<Container>(c))),
         end_iterator(adl_end(std::forward<Container>(c))) {}
   iterator_range(IteratorT begin_iterator, IteratorT end_iterator)
       : begin_iterator(std::move(begin_iterator)),
         end_iterator(std::move(end_iterator)) {}
 
   IteratorT begin() const { return begin_iterator; }