This is an archive of the discontinued LLVM Phabricator instance.

[Polly] Update ISL to isl-0.15-3-g532568a
ClosedPublic

Authored by Meinersbur on Jun 17 2015, 8:04 AM.

Details

Summary

This version adds small integer optimization, but is not active by default. It will be enabled in a later commit.
The schedule-fuse=min/max option has been replaced by the serialize-sccs option. Adapting Polly was necessary, but retaining the name polly-opt-fusion=min/max.

Diff Detail

Repository
rL LLVM

Event Timeline

Meinersbur updated this revision to Diff 27835.Jun 17 2015, 8:04 AM
Meinersbur retitled this revision from to Update ISL to isl-0.15-3-g532568a.
Meinersbur updated this object.
Meinersbur edited the test plan for this revision. (Show Details)
Meinersbur added a reviewer: grosser.
Meinersbur added a project: Restricted Project.
Meinersbur added subscribers: pollydev, Meinersbur, Unknown Object (MLST).
Meinersbur updated this object.Jun 17 2015, 10:23 AM
Meinersbur updated this object.
Meinersbur retitled this revision from Update ISL to isl-0.15-3-g532568a to [Polly] Update ISL to isl-0.15-3-g532568a.Jun 17 2015, 10:43 AM
grosser accepted this revision.Jun 17 2015, 10:40 PM
grosser edited edge metadata.

Hi Michael,

this patch looks good. I only have one remark:

It seems you somehow changed the format of isl_config.h. When importing isl I just copied this file from an isl_config.h.in. It seems you now use a slightly different approach to obtain the contents of this file which causes changes all over this file.
I understand that there may be better ways to generate this file, but I would prefer to not have the format change hidden within a larger 'isl-update' patch. Could we keep the original format for this patch?

If you have specific ideas how to improve this file, I would be interested to hear them, if possible, in a separate discussion.

Best,
Tobias

This revision is now accepted and ready to land.Jun 17 2015, 10:40 PM

It seems you somehow changed the format of isl_config.h. When importing isl I just copied this file from an isl_config.h.in. It seems you now use a slightly different approach to obtain the contents of this file which causes changes all over this file.

It is the output of ./configure on my machine.

Using isl_config.h.in straight away is about the least favorable thing you can do. It just defines nothing. I am surprised it worked.

It seems you somehow changed the format of isl_config.h. When importing isl I just copied this file from an isl_config.h.in. It seems you now use a slightly different approach to obtain the contents of this file which causes changes all over this file.

It is the output of ./configure on my machine.

Using isl_config.h.in straight away is about the least favorable thing you can do. It just defines nothing. I am surprised it worked.

The intention was to define as little as possible. My feeling was that isl would not use (or need) most of these macros anyhow, so I wanted to leave them undefined such that it clearly breaks if there arises a need at some point. In that case, we can then figure out how to derive them through cmake.

Using the configure of your machine just means it will work on your machine, but it seems there is a risk that your machine has certain features that are not available on other machines and consequently would break the build there if enabled.

As said before, I am happy to have this improved (and with your cmake-ificaton of isl you may have some good ideas), but let's not change our approach within a massive update of isl. Those two things are orthogonal.

Meinersbur updated this revision to Diff 27932.Jun 18 2015, 7:30 AM
Meinersbur edited edge metadata.

Use unconfigured isl_config.h

Meinersbur updated this revision to Diff 27933.Jun 18 2015, 7:34 AM

Accidentally uploaded a reverse patch last time

LGTM. Thank you!

It seems you somehow changed the format of isl_config.h. When importing isl I just copied this file from an isl_config.h.in. It seems you now use a slightly different approach to obtain the contents of this file which causes changes all over this file.

It is the output of ./configure on my machine.

Using isl_config.h.in straight away is about the least favorable thing you can do. It just defines nothing. I am surprised it worked.

The intention was to define as little as possible. My feeling was that isl would not use (or need) most of these macros anyhow, so I wanted to leave them undefined such that it clearly breaks if there arises a need at some point.

According to that logic we should use an empty isl_config.h

Using the configure of your machine just means it will work on your machine, but it seems there is a risk that your machine has certain features that are not available on other machines and consequently would break the build there if enabled.

It at least ensures that the macros are compatible with each other and work for gcc and clang. Using an unconfigured isl_config.h to work on any machine is pure coincidence.

As said before, I am happy to have this improved (and with your cmake-ificaton of isl you may have some good ideas), but let's not change our approach within a massive update of isl. Those two things are orthogonal.

Not really. isl_config.h is version-specific. Upgrading ISL logically also required upgrading isl_config.h

Michael

It seems you somehow changed the format of isl_config.h. When importing isl I just copied this file from an isl_config.h.in. It seems you now use a slightly different approach to obtain the contents of this file which causes changes all over this file.

It is the output of ./configure on my machine.

Using isl_config.h.in straight away is about the least favorable thing you can do. It just defines nothing. I am surprised it worked.

The intention was to define as little as possible. My feeling was that isl would not use (or need) most of these macros anyhow, so I wanted to leave them undefined such that it clearly breaks if there arises a need at some point.

According to that logic we should use an empty isl_config.h

That may indeed make sense.

Using the configure of your machine just means it will work on your machine, but it seems there is a risk that your machine has certain features that are not available on other machines and consequently would break the build there if enabled.

It at least ensures that the macros are compatible with each other and work for gcc and clang. Using an unconfigured isl_config.h to work on any machine is pure coincidence.

Maybe we have been lucky, right.

As said before, I am happy to have this improved (and with your cmake-ificaton of isl you may have some good ideas), but let's not change our approach within a massive update of isl. Those two things are orthogonal.

Not really. isl_config.h is version-specific. Upgrading ISL logically also required upgrading isl_config.h

Right. I was only concerned that we do this upgrade using the same approach as was used before, as we otherwise get random changes if different people update this file.

As I said, I am happy to have this improved. If you want to do so, feel free to start a discussion.

This revision was automatically updated to reflect the committed changes.