- User Since
- Jul 25 2016, 9:02 AM (168 w, 2 d)
Mon, Oct 14
Fri, Oct 11
Thu, Oct 10
LGTM. Please wait for a few days in case others have more comments.
Wed, Oct 9
I haven't looked at the patch in detail, but as author of at least part of the prior art cited here, I agree with the direction*. I also participated in some of the vector idioms discussions from a few years ago. There's overlap with the vector idiom problems, but as noted, these are generic (scalar too) math ops, so it's not exactly the same. We invested significantly in IR analysis and codegen for the math intrinsics, so that may have changed the thinking. I don't remember the sequence of events or if there was a dedicated llvm-dev thread for this, but the general idea is that if we have a generic intrinsic for the math and can easily invert the transform in the backend for targets/types that are not supported, try to canonicalize to the intrinsic.
Tue, Oct 8
Mon, Oct 7
Fri, Oct 4
>> In this regard, I do not like pass managers running Analyses for a Transformation pass w/o first letting the Transformation pass inspect the incoming IR, but that's a totally different discussion and I don't have a solution for that problem.
Maybe I'm misunderstanding, but SCEV is lazy so if a transform looks at the IR and decides not to ask SCEV for trip count or call getSCEV then SCEV will not do any actual work.
Vectorizer code change looks fine with me. I'd like to see the comments updated, though. Any more changes needed for the LIT tests?
I agree with @Ayal about changing getAsAddRec for OptForSize. In general, it's better to not "Analyze" if we know we won't be using the result of analysis.
Wed, Sep 18
Tue, Sep 17
Nice clean up of the code as well. LGTM.
Sep 9 2019
There are two ways to think.
- vectorize(disable) as in disable the LoopVectorize pass itself.
- vectorize(disable) as in disabling the loop vectorization transformation
Aug 6 2019
Buildbot failures reported so far. Just FYI. I don't think any actions from this commit is needed.
Aug 5 2019
Aug 2 2019
Reported buildbot fails for Evgueni to follow up as needed:
LIT test has been moved to X86 subdir, in response to ps4-buildslave1 buildbot failure.
Aug 1 2019
r367654 | hsaito | 2019-08-01 23:31:50 -0700 (Thu, 01 Aug 2019) | 12 lines
Jul 31 2019
Indeed, this is a much cleaner fix. LGTM.
LGTM, pending the discussion about the exact meaning of the newly introduced "vector predicate" pragma (expect this to happen outside of this review). Please wait for another day to give others last minute opportunity to give feedback.
Jul 29 2019
Jul 24 2019
We probably need to discuss whether vectorize_predicate(enable) should (or should not) implicitly turns on vectorize(enable) or not. I guess the current behavior is "does not", right? We don't have to discuss that in this review, but we still want to make a conscious decision one way or the other, or did I miss that discussion?
Just one comment from me.
Jul 19 2019
Looks to be a good change. Can we add a little more improvement to this patch --- adding more crispness in ORE message?
Jul 18 2019
Aha, that was a "hacky" way to get "loop contains a switch statement" along with the warning. I see. I suppose we can't blindly use LV_NAME, then.
I then suppose some tests (like pr38800.ll) didn't even need -pass-remarks-missed flag (which is incorrectly used, I think).
Jul 17 2019
Jun 25 2019
Looking at the expected output and the explanations on -Rpass* flags, it could be that those tests should be using -pass-remarks-analysis=loop-vectorize, instead of -pass-remarks-missed=loop-vectorize. Would you try?
Jun 21 2019
Jun 14 2019
Sorry, I missed the original review request in the e-mail pile up. Yes, this is the direction I was suggesting. Thank you.
May 30 2019
May 28 2019
May 24 2019
While we are looking at this, I'd like to discuss how to make these things easier. I think there a merit in using a utility function that takes three strings, something along the lines of
the following pseudo code:
May 9 2019
Apr 30 2019
Apr 29 2019
Apr 25 2019
Apr 24 2019
Apr 23 2019
Apr 17 2019
Apr 16 2019
Apr 15 2019
Apr 12 2019
Apr 5 2019
Mar 29 2019
Mar 28 2019
Mar 27 2019
Mar 25 2019
@hsaito, I don't have commit access, could you commit this change for me?
Mar 22 2019
Mar 19 2019
Mar 14 2019
LGTM. Please wait for a few days to give others a chance to go over your updated patch.
Mar 13 2019
Mar 8 2019
Feb 6 2019
I have a little mental barrier in accepting this change as is. I think this feeling of mine is mainly due to the name and the "stated" functionality of computeMaxVF and "indirect inference" towards using it for suppressing interleaving. Maybe I'm just being too picky here. If so, my apologies ahead of time.
Feb 5 2019
Feb 1 2019
While we are at this, let's talk about downstream dependency, if any, for allowing more than one candidate VF along the native path. At least we can write down a list of TODOs so that we'll be aware of the things we need to improve at a time in the future.
Jan 31 2019
Jan 23 2019
Dec 19 2018
Unfortunately, this could not be done with computeInstDiscount() as a simple extension of (1), since memory accesses are dealt with before anything else.