- User Since
- Jan 26 2016, 2:17 AM (218 w, 7 h)
Ah, I now see that Dave commented on the other ticket, that probably needs to be addressed first...
LGTM (perhaps the same nit as in the other review)
LGTM, with one nit that doesn't need another review.
Rewrite using new function reverseInductionVariable in LoopInfo, see also the updated Title/Summary.
Ah yes, it's getCallCost calling getIntrinsicCost, and this is about getCallCost, so agreed.
This is a tricky one, because usually it is quite simpel: if code is not used, it should be deleted. However, estimating costs of e.g. libc functions is such a glaring omission, something we definitely should be doing, but we aren't very unfortunately. I had to switch tasks before I could finish D59129, but that is just one example how we could estimate costs.... So yeah, I don't know, if this is really bothering you, we can get rid of rid, but perhaps keeping this infrastructure isn't that bad?
Perhaps this is also a good motivation to pick up D59129 again on the side...
Fri, Mar 27
Cheers, with some nits addressed this looks good.
A few irrelevant nits inlined, but also one question about the test.
Looks reasonable to me.
Thu, Mar 26
Thanks for commenting!
2-character changes that fix wrong codegen are always good changes, so have no problem directly LGTM'ing this.
Will do, and many thanks for all your reviews!
- replaced ID.getStep() -> Step
- removed the sentence on line 1899 - 1900, and
- moved the comments on lines 1900 - 1904 to 1889.
Wed, Mar 25
Many thanks for reviewing!
I think this has all comments addressed.
Cheers, nice one.
Thanks for that example! I asked this question because I expected the vmax and vmin to behave roughly the same. In your example, if you change the input and example from 0x00010203 to 0x04010203, then the VMAX will also give a different result after tail-predication, or am I still missing something?
Tue, Mar 24
Did a first scan, looks very reasonable, just some first nits/questions inlined.
Yes you're right, the patch that I've made dependent needs this one to work properly.
The goal of this pass is to maximize the size of the VPT blocks created by the MVE VPT Block Insertion pass.
The unit test failure looks genuine, does that needs fixing?
Thanks for taking a look @fhahn ! Comments addressed.
Patches with functional changes but without tests are a bit "suspicious". In this case, I see it might be tricky. You could argue that it should be incorporated in the patch that includes the tests, so we can see what's happening. But perhaps separating this out is cleaner, if the other patch is big. But can you at least make the next patch dependent on this one, so we know where this is used?
Mon, Mar 23
Cheers, looks good to me.
Looks like a good direction to me.
Fri, Mar 20
Looks good to me. Please wait a day in case Eli has more comments.
Thu, Mar 19
I am now up to date with this, and don't have any questions about the logic, that looks good.
I would like to go over it once more when (some of) the nits have been addressed as I would like to get an overview how things read and look as this introduces quite a few new things and concepts.
Besides the irrelevant formatting nits, one minor question about the clang test.
Some nits inlined
Wed, Mar 18
That sounds good and that seems to address the feedback given here, which I agree with. Just wanted to briefly add that while it already looks like most interested people are on this ticket, perhaps it good to also share the plan and design on the cfe dev list for visibility.
Mon, Mar 16
Big patch: this is just a first scan of the code, and a first round of nits. Now going to look again, to let things sink in.
Thu, Mar 12
I'm still not sure why __fp16, which is a storage-only type, is used for the element type of float16x4_t if we want to avoid promotion to a float vector type.
Wed, Mar 11
With the nit addressed, this LGTM
Tue, Mar 10
Cheers, I think this looks very reasonable.
LGTM, with one nit inline.
Mon, Mar 9
was just wondering about more corner cases for "negative" tests: do we have test where lanes are swapped (if that makes sense)?
Thu, Mar 5
I wanted to include these test changes in the recommit, but perhaps it's nice to commit this separately (as this this a change in a different component).
So wanted to check if we are happy with this?
Adding @simon_tatham in case he feels wants to have a look too.
Agreed, they don't swap lanes. They can write to only bottom/top halfs, but that's fine. So looks like a straightforward change to me.
Wed, Mar 4
Big patch, I only did a first scan, but looks very reasonable in general. Just a first round of nits and 2 questions.
Thanks for that solution, have copied it to the test file.
Tue, Mar 3
Patch for the test-suite: D75557
Thanks James, I will prepare a new revision!
The test-suite needs porting too (see comment in D75056) and am first looking into that, after that I will return to this.
Ha, these test-suite apps fail with multiple definition of ... link errors, so actually require a little bit of porting! :-)
But I think I will prepare a patch that adds -fcommon to their makefiles, as I don't want to change too many things at the same time.
Reverted in: https://reviews.llvm.org/rG4e363563fa14
I probably forgot to add why these tests started failing!
Mon, Mar 2
Thanks all for the reviews and help.
I will fix these things and do a last rebase/check before committing this tomorrow.
Nice optimisation, looks very reasonable to me: LGTM
Friendly ping, and just checking: are we happy with this? Good to go?
Feb 28 2020
Looks like this needs a rebase?
Feb 27 2020
Thanks for that! That greatly helps readability!
Are there any tests remaining that check that with -fcommon, IR generation creates "common" variables, now that all these tests have been modified?
That should happen automatically when you put the tag Differential Revision: ... in the commit message.
It doesn't look like a made spell mistake in it, so am a bit puzzled why this didn't happen.... but anyway, it's closed now.
Sure, no problem, this is committed in: https://github.com/llvm/llvm-project/commit/13db7490fa67e22605dec4ab824121230b0fd928
Feb 26 2020
Removed the CHECK-NOTs from some tests as suggested.
Yep, those are the instructions to request commit access, which might be good to request if you plan to do more llvm work. I guess that will probably take a few days. And if you're not in a hurry with getting this committed, you could wait for your commit access and then commit this yourself. But I would of course be happy to commit this on your behalf, which I will then do tomorrow morning, just let me know what you prefer.
minor test update
Thanks for catching that. Now it shows more changes in tests, so updated a bunch of tests.