- User Since
- Jan 26 2016, 2:17 AM (281 w, 6 d)
Yep, looks like a good test to me.
Nice one. This work was motivated by a missed opportunity in the SLP vectoriser.
Can you separately create a reduced test case for that, so that we can first precommit that?
Then, that tests should be modified here.
Specialising on constant integers would definitely be good, and is indeed what we want. But it's early days for function specialisation, and there are quite a few limitations, and this is just one of them.
LGTM, but please wait a day with committing this in case there are more comments.
Let me explain why I am a bit pushing back at this: I would like to see function specialisation enabled by default one day, and therefore I am interested to know how much more difficult that is going to be when we also specialise on integer constants. In previous comments you mentioned:
Thu, Jun 17
Wed, Jun 16
Fixed typo in test.
But I guess it may be OK since we control the cost to specialize function incrementally (implemented in getSpecializationCost()). So the amount of new specialized functions wouldn't be much larger.
Supporting integer constants opens up a whole new set of opportunities for function specialisation. This will have a big impact on compile-times, which is the main concern of this pass.
I suggest we take one step at a time, and add support for integer constants under an option that is off by default. Just for testing purposes this would be convenient, but it also allows to measure the impact on compile-times by toggling this option on/off.
Besides some nits/observations inline, one question about testing. The reproducer that was attached does not get miscompiled anymore? And it is exactly one of these cases that was added, i.e. the modified base register? Just asking to see if there's not another case or test that we are missing.
Tue, Jun 15
This is now only about statistics: commented/renamed the counter, and added a test case.
Thanks for your comments. I quickly put this patch together to collect post-commit comments. I will pick this is up today.
Fri, Jun 11
The wrong location didn't sit right, so I have precommitted that in rG9907746f5db7 before I continue with the rest and this is a little rebase on top of that.
This is sort of a placeholder for follow up comments of D93838 while I am looking into a few things.
Cheers, looks like a good change to me.
Many thanks for all your help and reviews. I have committed this because it was lgtm'ed (officially and also unofficially in comments), no major changes have been made/suggested for a while, and this is a first version that is off by default. And this is also just the beginning of function specialisation. I will continue working on this and will be happy to receive post-commit comments for this version, and of course comments for the work that I am planning and starting to do. I.e., I will now start working on some of the limitations before I start thinking how to eventually get this enabled by default.
Thu, Jun 10
Many thanks for the reproducer!
We are going to have a look at that.
Wed, Jun 9
Removed those updates too, added the test case back. Appreciate it if you can have one more quick look.
Hmm, strange some comments went missing when I pushed the button previously.
Perhaps you can elaborate a little bit in the commit message:
Tue, Jun 8
Mon, Jun 7
For our use cases, it's more interesting to see how much benefit we could get from non-function-pointer constants, on top of PGO
Fri, Jun 4
Perhaps for bonus points, update the Clang documentation in https://clang.llvm.org/docs/LanguageExtensions.html#matrix-types with some examples?
Thu, Jun 3
Sorry, ignore my previous comments, I have spotted them now (with your help).
Wed, Jun 2
Hi Florian, thanks for putting this up for review and starting the discussion. If you don't mind me asking, how high is is on your todo-list? The reason I am asking is that this well help x264 where we are behind a lot (and it more in general it solves this long outstanding issue that we have). Fixing x264 is high on our list, which is why I put up D102748 to start this discussion. If, for example, you don't see time to work on this, we could consider looking at it.
Thanks for raising that one. It feels like the number of missed opportunities is endless. From the discussion here however, my impression was that there's very little appetite to skip the full unroller at this point and that running the LV before the full unroller would involve quite some surgery in the optimisation pipeline. My take away was that trying to improve the SLP was probably more low hanging fruit. Thus, I was thinking about abandoning this change....
Addressed @fhahn 's comments: don't run the solver for specialised functions, removed the recursive specialization test for now.
Tue, Jun 1
Thu, May 27
Sorry for the delay. Looks reasonable to me.
Wed, May 26
Tue, May 25
Mon, May 24
This addresses most remarks:
- transitioned to using InstructionCosts,
- added tests/testing,
- removed unnecessary things.
May 20 2021
Ah cheers, many thanks for the comments and review! Will start addressing them now.
Thanks, interesting, having a look there!
Yes, interesting. LoopVersioningLICM came to my mind, except that works on loops of course...
Hello, this work was brought to my attention in D102748. Would be good to get this in, and seems almost finished too. Any plans to pick this up? Otherwise I think I would be interested in doing that.
Thank you for further discussing this.
May 19 2021
Thanks for those examples. I am going to play a little bit more with this.
I don't have suggestion on how to move forward *this* patch, because I think it's fundamentally the wrong direction to take.