Lgtm, thank you.

This makes a lot of sense to me too.

I think this is a great idea.

I just checked and I get the same error on a linux system. @nhaehnle, did you try an mlir build with the previously mentioned cmake options?

This patch overcomes the issue of cl options being defined multiple times. That is great!

This is excellent. Thank you Michael!

This looks good from my side. However, maybe Michael Kruse has further remarks?

Indeed. If no test case is provided, there should be an explanation in the commit message why providing a test case is impractical.

Hi @arjunp, I am wondering if there are ways to prevent future coverity warnings on this code?

Maybe the windows build is just broken. If this is clearly unrelated, I would not hold this patch back.

There is a typo in the commit message: deteectRedundant

This looks good from my side as well. Let's see what @rriddle says.

Amazing. Congratulations that you identified this issue!

This looks good to me.

The update of the commit message is enough, from my side.

Is there a way to add a test case for this?

This looks good from my side as well. Given that these are rather straightforward extensions to the API as well as much appreciated polishing of some simple functions, you can probably go ahead with committing them.

This looks also good from my side. Given that this is a very simple and non-algorithmic change you can probably go ahead with committing this.

@Groverkss, the commit message still references the bugfix that was factored out.

I feel that this change can be reviewed post-commit following this policy: https://llvm.org/docs/CodeReview.html#can-code-be-reviewed-after-it-is-committed.
In the unlikely case that this purely mechanical case causes concerns, we can easily revert it and start a full patch review.

LGTM

This looks good from my side.

This looks reasonable from my side.

This looks like a better solution to the earlier reported bug. Thanks @EliaGeretto for working on this! I like the general approach. I only have two minor suggestions: (a) add some comments -- especially one that explains why the correct removal order is important for external plugins, (b) consider replacing an emptiness check with a targetted assert.

@EricWF and @timshen, @maxf and I are indeed very interested in seeing this library upstreamed soon. Over the last weeks, we developed a software prototype based on these bindings and already started to tune performance on clang/LLVM. As part of this, we looked into efficient vector code generation, started to implement where expressions, ... Currently, everything is very much prototype quality, but we would very much prefer to get things integrated into libcxx. For this, it's important to have a working and committed implementation, such that we can start submitting patches ourselves and can obtain feedback on these patches early on. Otherwise, we might take directions that won't really be compatible with libcxx. How can we best help with getting these patches upstream?

Nice. Btw. another motivation could be std::simd. Here the overflow intrinsics exposed as builtin would allow us to provide a fast implementation of the masked variants of <simd>

@EricWF thanks again for offering your help here. We are not in a rush here, but I wonder if you happen to have a rough estimate when you might have the time to look into these patches?

In D41412#1592152, @timshen wrote:

Tobias,

I spoke to @EricWF who is willing to take a look at all these patches. However, I don't know when exactly will the review starts.

It seems this patch went through at least one review and the only open comment is the discussion if __builtin_shuffle should be placed in the configuration. From my perspective, both solutions are technically feasible. While it seems unlikely that gcc will gain this specific builtin, I can see @mclow.lists preferring to be consistent in keeping compiler-specific stuff in one file rather than having to evaluate for each extension the likeliness of this extension being adopted over time.

I am very interested in these patches. Any chance you can take up the upstreaming process again?

Tim's patches look a lot more polished and are better in line with the initial ::simd code he contributed. I feel development should be continued on Tim's patches.

In D64702#1586229, @timshen wrote:

a) tim has already a similar patch in his pipeline

Yes, I had a full stack of patches (using the dependency feature of Phab), starting with D41412, that implements the whole proposal up to some version (and it's not the latest version), but the patches got into both mclow's and my back-burners sooner than expected.

LGTM elli

Hi Michael,

Marcin, let us know if you agree with the comment fix. I can then commit the patch, when it's ready.

Thanks Marcin for improving the commit message. We all seem to agree that this is the right correction. Hence, I feel this is ready to upstream from my side. As there have been quite some opinions, it would be great to hear if there are any open concerns that should be adressed or if others also feel this LGT(them). We especially still need feedback from @kristina!

Thank you @kristina for the review. I must admit I do not fully understand in which way this is broken, but the attached test case clearly shows there is some memory issue. My (limited) understanding is that Twine(Weight->getZExtValue()) will be destroyed at the end of line 175 which means that already in line 176 we may potentially reference invalid memory.
http://llvm.org/docs/ProgrammersManual.html#dss-twine has a similar example and explicitly warns of such uses:

Thank you guys for taking care of some of the cmake system. @Meinersbur, I am fine with renaming Polly modules according to LLVM conventions if this makes things more consistent.

This is a good idea.

This LGTM.

Interesting. Any idea what is going wrong here?

Hi Michael,

If this was not clear. I am very OK with this change.

This is a straightforward, but very nice infrastructure change. Thank you both for pushing JSON support ahead in LLVM!

http://brotherst-code.blogspot.com/2015/10/how-to-get-typeindex-without-rtti.html

I used the std::any variant as an easy way to showcase how the bindings generator could be modified to do what we want (and also to show how an ideal interface could look like). I feel we should bring this up on the isl mailing list first to see what would be a good way to add this feature. We potentially could replace std::any with a simpler C++11 compatible solution:

Hi Philip.

LGTM

This has become obsolete, after Philip Pfaffe committed a better variant of this feature.

You can commit this. It will just be covered in the next update.

Sorry guys. Alain just started to look into this last week and we wanted to get him up-to-speed before getting in touch with you. I should have helped to coordinate better. I asked Alain to discuss with you which / what patches are still needed.

I have a patched version of the generator (very hacky) on top of the isl bindings. Not sure if this (happens to) be the right one: https://github.com/tobig/isl/tree/polly-bindings-andrei

Right. I am busy today, but can do a normal update thursday afternoon. I will then also push the current generator to github.

LGTM

LGTM.

Yes, raise we should raise the version. Also, please add a propoer commit message explaining what the underlying error was here. We should also make sure that this bug has been fixed in the latest ppcg, such that we do not re-import it again.

Also looks good from me except the minor changes. Please update the patch if you agree.

@ftynse, the source is referenced in the patch summary. If we commit such a patch to Polly, we must make we have your permission to contribute it under the LLVM license and obviously need to acknowledge the source. For now, I asked Lorenzo to push the code as RFC to allow others to see how it would be used. This already sparked a comment from Philip and will likely spark further discussions on how to extend the interface. Thanks for commenting.

Nice!