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

[Tobias Grosser via llvm-dev]

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?

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