- User Since
- Jul 30 2014, 11:40 AM (452 w, 1 d)
Wed, Mar 8
Lgtm, thank you.
Jan 26 2023
This makes a lot of sense to me too.
Jan 25 2023
I think this is a great idea.
Nov 21 2022
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!
May 9 2022
This is excellent. Thank you Michael!
May 8 2022
This looks good from my side. However, maybe Michael Kruse has further remarks?
May 1 2022
Indeed. If no test case is provided, there should be an explanation in the commit message why providing a test case is impractical.
Apr 13 2022
Feb 24 2022
Feb 18 2022
Jan 5 2022
Hi @arjunp, I am wondering if there are ways to prevent future coverity warnings on this code?
Nov 29 2021
Nov 26 2021
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
Nov 25 2021
Nov 15 2021
This looks good from my side as well. Let's see what @rriddle says.
Nov 14 2021
Nov 13 2021
Amazing. Congratulations that you identified this issue!
Nov 8 2021
This looks good to me.
Oct 17 2021
The update of the commit message is enough, from my side.
Is there a way to add a test case for this?
Sep 17 2021
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.
Sep 3 2021
Aug 4 2021
@Groverkss, the commit message still references the bugfix that was factored out.
Jul 19 2021
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.
Apr 2 2020
Feb 27 2020
Jan 3 2020
This looks good from my side.
This looks reasonable from my side.
Dec 29 2019
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.
Aug 17 2019
@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?
Aug 9 2019
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>
Jul 25 2019
@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?
Jul 19 2019
Jul 18 2019
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.
Jul 15 2019
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.
Jul 14 2019
May 10 2019
Apr 23 2019
Feb 1 2019
Oct 8 2018
Marcin, let us know if you agree with the comment fix. I can then commit the patch, when it's ready.
Oct 7 2018
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:
Sep 13 2018
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.
Sep 11 2018
This is a good idea.
Sep 4 2018
Aug 8 2018
Aug 6 2018
Aug 1 2018
Jul 31 2018
Interesting. Any idea what is going wrong here?
Jul 28 2018
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!
Jul 25 2018
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:
Jul 20 2018
Jul 17 2018
This has become obsolete, after Philip Pfaffe committed a better variant of this feature.
Jul 16 2018
Jul 13 2018
You can commit this. It will just be covered in the next update.
Jul 10 2018
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.
Jul 6 2018
Jul 5 2018
Jul 4 2018
Jul 3 2018
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.
Jun 29 2018
Also looks good from me except the minor changes. Please update the patch if you agree.
Jun 28 2018
@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.
Jun 26 2018