<div dir="ltr">Hi Micheal,<div><br></div><div>thanks for moving this forward!<br><div class="gmail_extra"><br><div class="gmail_quote">2018-01-15 17:52 GMT+01:00 Michael Kruse via llvm-dev <span dir="ltr"><<a href="mailto:llvm-dev@lists.llvm.org" target="_blank">llvm-dev@lists.llvm.org</a>></span>:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Dear community,<br>
<br>
for our goal to make polyhedral optimization available in the main<br>
LLVM source, we will need the Integer Set Library (isl)[1]. It is the<br>
main dependency of Polly, but would be required even if we do not<br>
directly import Polly.<br>
<br>
I already prepared a patch [2], unfortunately without feedback on the<br>
general approach. As Philip suggested in the review, I am reaching out<br>
with an RFC with a wider audience on llvm-dev.<br>
<br>
As for the main motivation on why to import the entire source of isl at all:<br>
Polly interacts relative tightly with isl which provide the main<br>
optimization algorithms. For instance, Polly's regression tests<br>
compare the string output of set representation, which unfortunately<br>
can change between versions. For instance, if a newer version<br>
implements a simpler string description of the same set. That means<br>
that tests would fail if a different version is installed on the host<br>
system. This is significant for buildbots that would need regular<br>
manual action to install a newer version of isl on the system.<br>
<br>
An alternative would be to only store the git sha1 and a CMake script<br>
would download that version when building. However, it would also be<br>
more difficult to both sources in-sync in the checkout (think of git<br>
bisect).<br>
<br>
I suppose these were also the reasons why Polly imported the isl<br>
source in commit r228193 instead of requiring users to call<br>
utils/checkout_isl.sh manually.<br>
<br>
<br>
The other design decisions and rationales are mentioned in the review<br>
summary [2]. I repeat them here so they can be referred to in replies.<br>
<br>
* The library is named LLVMISL and contained in the lib/ISL folder to<br>
work best with LLVM's component system. The component's name "ISL" was<br>
chosen over "isl" as it matches the capitalization of other<br>
two/three-letter-acronym components.<br>
<br>
* Isl's headers are installed into $PREFIX/include/llvm/ISL/isl. This<br>
is to not conflict with potentially user/system-installed isl headers.<br>
Please note that this means that MIT-licensed headers land on the<br>
target system.<br></blockquote><div> </div><div>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?</div><div><br></div><div>Cheers,</div><div>Philip</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br>
* The source of isl itself is added to the contrib/isl directory. This<br>
is to keep the source in one directory rather than spreading it into<br>
the lib and include directories, facilitating integration of upstream<br>
changes. There is a script "isl-merge.py" that automates this. It<br>
currently requires LLVM being checked out using git. The new subfolder<br>
"contrib" should make it clear that this is downstream code and maybe<br>
one shouldn't run clang-format over it which would make the next merge<br>
difficult.<br>
<br>
* There is an additional "GIT_HEAD_ID" file containing the upstream's<br>
revision. It is used by isl-merge.py to determine the common ancestor<br>
and CMakeLists.txt to determine the version. Normally isl's make dist<br>
would generate this file, it is done manually here to not requiring a<br>
configure/make/make dist cycle when merging the latest isl. Due to<br>
different Autotools versions being installed on the different systems,<br>
this might lead to spurious conflicts with the generated configure<br>
etc. files. However, due to the need of an isl-noexeceptions.h, I may<br>
need to include such a cycle in isl-merge.py anyway.<br>
<br>
* Isl has its own unittest "isl_test.c". It is put into bin/ (but not<br>
installed) and run by llvm-lit on llvm-check. This was just the<br>
simplest to do since it doesn't require any additional lit.site.cfg in<br>
order to find the isl_test in a different directory.<br>
<br>
<br>
Looking forward for your feedback.<br>
<br>
Michael<br>
<br>
<br>
[1] <a href="http://isl.gforge.inria.fr/" rel="noreferrer" target="_blank">http://isl.gforge.inria.fr/</a><br>
[2] <a href="https://reviews.llvm.org/D40122" rel="noreferrer" target="_blank">https://reviews.llvm.org/<wbr>D40122</a><br>
______________________________<wbr>_________________<br>
LLVM Developers mailing list<br>
<a href="mailto:llvm-dev@lists.llvm.org">llvm-dev@lists.llvm.org</a><br>
<a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/<wbr>mailman/listinfo/llvm-dev</a><br>
</blockquote></div><br></div></div></div>