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

[Michael Kruse via llvm-dev]

Thank you for the feedback.

2018-01-15 18:21 GMT+01:00 Hal Finkel <hfinkel at anl.gov>:
> If this is ready to be reviewed, I recommend removing the "[WIP]" from
> the title. Normally I interpret "[WIP]" (which I presume stands for
> "work in progress") as meaning "I'm looking for early feedback on this
> approach, perhaps primarily from a limited audience, but the patch
> itself is not really ready for review (e.g., still known not to build,
> to crash, miscompile, and so on).

Thanks for the hint, I did not think of this. The intention to keep
the flag was that I do not intend to commit it right away, but rather
to discuss the approach. Committing is only becomes necessary when
implementing something requiring it.


>> * 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.
>
> Can you clarify when/if autotools are required and under what circumstances.

Polly's update_isl.py runs it to generate the files GIT_HEAD_ID,
isl_config.h and isl-noexeceptions.h where only the GIT_HEAD_ID is
actually used.

GIT_HEAD_ID:
Polly) Use the one generated by autotools, run by update_isl.py
D40122) Generated by isl-merge.py

isl_config.h:
Polly) Generated by CMakeLists.txt
D40122) Generated by CMakeLists.txt

isl-noexeceptions.h:
Polly) Added manually to the repository. When an update is necessary,
the user has to configure/build isl and copy the generated
isl-noexeceptions.h into the repository
D40122) There is no isl-noexeceptions.h

Since sometime we will need isl-noexeceptions.h, here are the approaches:

using autotools) Like update_isl.py, run autotools by isl-merge.py and
copy the file generated into the repository.
without autotools) Run the commands that the autotools' build would
run by isl-merge.py

The latter might require maintenance if the way isl's build system
creates isl-noexeceptions.h changes.

autotools are required only in the former solution and only when
someone wants to merge a newer version of isl by running isl-merge.py.


>> * 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.
>
> Can it end up in the unittests subdirectory with the other unit tests?

The lit.cfg in test\Unit directory assumes that all tests are using
gtest. This is not the case for isl_test.

The unittests subdirectory only contains the sources of the unittests,
isl_test's sources are in contrib/isl/isl_test.c for the
external-source separating reason.

We could move the CMakeLists.txt to build isl_test into the unittest
dir, but for compiling, it requires some definitions from
lib/ISL/CMakeLists.txt (e.g. include paths) which are not available in
sibling build directories. One could move these definitions further up
into the root CMakeLists.txt or duplicate in unittests/ISL. IMHO, both
have more downsides than the advantage of having a unittest/ISL
directory.


Michael