[llvm-dev] RFC: Import of Integer Set Library into LLVM source tree

Tue Jan 16 15:26:34 PST 2018

Hi folks,

I actually just wanted to point out that this means isl will also be part
of any binary distribution. Especially if, as Tobias says, minor
differences can lead to issues. In that, I'm not worried about the LLVM
community, since they have the in-tree version available. But do we want to
throw that onto the average, say, Ubuntu user?

Alternatively, we can pull isl from the public Polly interface, and
possibly replace it with a wrapper that we can ship. I'm worried that that
might incur a performance impact, but of course we'd have to see numbers on
that.

Cheers,
Philip

2018-01-16 18:04 GMT+01:00 Hal Finkel <hfinkel at anl.gov>:

>
> On 01/16/2018 01:59 AM, Tobias Grosser via llvm-dev wrote:
> > On Tue, Jan 16, 2018, at 08:53, Tobias Grosser via llvm-dev wrote:
> >> On Mon, Jan 15, 2018, at 22:10, Philip Pfaffe via llvm-dev wrote:
> >>> Hi Micheal,
> >>>
> >>> thanks for moving this forward!
> >>>
> >>> 2018-01-15 17:52 GMT+01:00 Michael Kruse via llvm-dev <
> >>> llvm-dev at lists.llvm.org>:
> >>>
> >>>> Dear community,
> >>>>
> >>>> for our goal to make polyhedral optimization available in the main
> >>>> LLVM source, we will need the Integer Set Library (isl)[1]. It is the
> >>>> main dependency of Polly, but would be required even if we do not
> >>>> directly import Polly.
> >>>>
> >>>> I already prepared a patch [2], unfortunately without feedback on the
> >>>> general approach. As Philip suggested in the review, I am reaching out
> >>>> with an RFC with a wider audience on llvm-dev.
> >>>>
> >>>> As for the main motivation on why to import the entire source of isl
> at
> >>>> all:
> >>>> Polly interacts relative tightly with isl which provide the main
> >>>> optimization algorithms. For instance, Polly's regression tests
> >>>> compare the string output of set representation, which unfortunately
> >>>> can change between versions. For instance, if a newer version
> >>>> implements a simpler string description of the same set. That means
> >>>> that tests would fail if a different version is installed on the host
> >>>> system. This is significant for buildbots that would need regular
> >>>> manual action to install a newer version of isl on the system.
> >>>>
> >>>> An alternative would be to only store the git sha1 and a CMake script
> >>>> would download that version when building. However, it would also be
> >>>> more difficult to both sources in-sync in the checkout (think of git
> >>>> bisect).
> >>>>
> >>>> I suppose these were also the reasons why Polly imported the isl
> >>>> source in commit r228193 instead of requiring users to call
> >>>> utils/checkout_isl.sh manually.
> >>>>
> >>>>
> >>>> The other design decisions and rationales are mentioned in the review
> >>>> summary [2]. I repeat them here so they can be referred to in replies.
> >>>>
> >>>> * The library is named LLVMISL and contained in the lib/ISL folder to
> >>>> work best with LLVM's component system. The component's name "ISL" was
> >>>> chosen over "isl" as it matches the capitalization of other
> >>>> two/three-letter-acronym components.
> >>>>
> >>>> * Isl's headers are installed into $PREFIX/include/llvm/ISL/isl. This
> >>>> is to not conflict with potentially user/system-installed isl headers.
> >>>> Please note that this means that MIT-licensed headers land on the
> >>>> target system.
> >>>>
> >>> I think this needs to be emphasized. This means we effectively ship
> another
> >>> version of isl to our users, independent of whether they have one
> already.
> >>> Is this really what we want?
> >> Yes, I feel this is important for predicability and reproducability of
> >> test cases. Even minor differences in isl version might result in
> >> differences in how code is compiled. Without using a fixed version of
> >> isl for a given SVN id, it will be more difficult to reproduce bug
> >> reports.
> >>
> >> Do you see any drawback with this approach?
> > Also, to add some more context. gcc e.g. does not include isl as a
> dependency, but a limited set of isl versions can be chosen. So this is
> indeed possible. However, it the set of isl versions that are allowed is
> indeed rather limited, and besides the previously mentioned drawbacks, we
> also have less test coverage on combinations that have not been validated
> for a release and it is harder to get fixes fast into isl (if older
> releases should still somehow be supported).
> >
> > I feel the LLVM community as a whole has a tradition of 1) staying on
> trunk and 2) including all (most) dependences in the default checkout.
>
> I completely agree for exactly these reasons. It also gives us greater
> flexibility in how we move forward with the integration, for example how
> things are refactored, later on in the process.
>
>  -Hal
>
> >
> > Best,
> > Tobias
> >
> >> Best,
> >> Tobias
> >>
> >>> Cheers,
> >>> Philip
> >>>
> >>>
> >>>> * The source of isl itself is added to the contrib/isl directory. This
> >>>> is to keep the source in one directory rather than spreading it into
> >>>> the lib and include directories, facilitating integration of upstream
> >>>> changes. There is a script "isl-merge.py" that automates this. It
> >>>> currently requires LLVM being checked out using git. The new subfolder
> >>>> "contrib" should make it clear that this is downstream code and maybe
> >>>> one shouldn't run clang-format over it which would make the next merge
> >>>> difficult.
> >>>>
> >>>> * There is an additional "GIT_HEAD_ID" file containing the upstream's
> >>>> revision. It is used by isl-merge.py to determine the common ancestor
> >>>> and CMakeLists.txt to determine the version. Normally isl's make dist
> >>>> would generate this file, it is done manually here to not requiring a
> >>>> configure/make/make dist cycle when merging the latest isl. Due to
> >>>> different Autotools versions being installed on the different systems,
> >>>> this might lead to spurious conflicts with the generated configure
> >>>> etc. files. However, due to the need of an isl-noexeceptions.h, I may
> >>>> need to include such a cycle in isl-merge.py anyway.
> >>>>
> >>>> * Isl has its own unittest "isl_test.c". It is put into bin/ (but not
> >>>> installed) and run by llvm-lit on llvm-check. This was just the
> >>>> simplest to do since it doesn't require any additional lit.site.cfg in
> >>>> order to find the isl_test in a different directory.
> >>>>
> >>>>
> >>>> Looking forward for your feedback.
> >>>>
> >>>> Michael
> >>>>
> >>>>
> >>>> [1] http://isl.gforge.inria.fr/
> >>>> [2] https://reviews.llvm.org/D40122
> >>>> _______________________________________________
> >>>> LLVM Developers mailing list
> >>>> llvm-dev at lists.llvm.org
> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >>>>
> >>> _______________________________________________
> >>> LLVM Developers mailing list
> >>> llvm-dev at lists.llvm.org
> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> >> _______________________________________________
> >> LLVM Developers mailing list
> >> llvm-dev at lists.llvm.org
> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
> > _______________________________________________
> > LLVM Developers mailing list
> > llvm-dev at lists.llvm.org
> > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
> --
> Hal Finkel
> Lead, Compiler Technology and Programming Languages
> Leadership Computing Facility
> Argonne National Laboratory
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180117/0cafb903/attachment.html>