llvm dev - Jan 2018 - RFC: Import of Integer Set Library into LLVM source tree

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

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.

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

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