- User Since
- Jul 25 2016, 9:02 AM (235 w, 2 d)
Jan 14 2020
We shouldn't make this either/or. Ability to runtime check unit-stride is good, and ability to use gather/scatter is also good. Depending on the target, I see the following situations
- don't vectorize the loop ---- unit-strided code alone isn't profitable or loop itself is profitable but not good enough to cover runtime check cost.
- vectorize with runtime check, with scalar code to fall back ---- when gather/scatter code is deemed not profitable.
- vectorize with runtime check with gather/scatter code to fall back
- vectorize with gather/scatter
Dec 2 2019
Nov 21 2019
This patch looks good to me. I agree it's nicer if we could fix it during widenInstruction, but let's leave that fix to the VPlan centric code generation, where we expect to (eventually) have the necessary context.
Nov 8 2019
Are we essentially saying that any reassociation can't preserve NSW/NUW flags?
Nov 1 2019
We have observed some performance regressions, presumably because the vectorized code started to kick-in on short estimated trip count loops (as opposed to skipping vector code and execute scalar code). We'll try following up with cost model tuning. I'm not too surprised if others also hit a similar issue. Overall, though, that's the right direction to head to.
Oct 28 2019
Oct 25 2019
Oct 18 2019
Oct 17 2019
Have we established general consensus for the desire to have the flexible enough loop optimization pass ordering to accomplish the outcome of the new directive, and shared vision for the path to get there? If we are making this a general clang directive, I'd like to see the vision to get there w/o depending on polly. If this is already discussed and settled, pointer to that is appreciated so that I can learn.
@Meinersbur, if I remember correctly, there was an RFC discussion on this topic, right? If yes, would you post the pointer to that? I need a refresher on what has been discussed/settled in the past.
Oct 16 2019
Oct 14 2019
Oct 11 2019
Oct 10 2019
LGTM. Please wait for a few days in case others have more comments.
Oct 9 2019
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.
Oct 8 2019
Oct 7 2019
Oct 4 2019
>> 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.
Sep 18 2019
Sep 17 2019
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.