As far as I can see, there is no good reason for the special treatment of vector bool/pixel going forward. Could we drop this special treatment, or at least change the default to use a scalar results across the board (consistent with XL's behaviour and clang's current behaviour for most cases).

LGTM

I like this change, but some of the widening decisions used to feed back to the result of isScalarWithPredication which may alter the result in unexpected ways. I'd suggest you try to remove the VF from that function in a separate patch first. If it doesn't break anything anywhere, then we can move ahead with this patch.

I'm not exactly sure what it means to "access memory that is not accessible by the module being compiled.". My guess is that it's for things like intrinsics that take a global string as argument (eg. the file name), for compile-time mapping, but don't get lowered to any instructions that actually access that memory in the final generated assembly. Is that correct, or are there other examples to consider?

LGTM, with a minor nit-pick.

So if I understand correctly, we still allow code in the exit block and the other target of the guard branch like before, so this is purely an improvement over what we had before. Although, as I said before I think we should allow non-empty blocks as well, but that can come as future extensions.

In D94717#2737167, @Whitney wrote:

As you can see for.end is not empty and so this loop's guard won't be detected!

From looking at skipEmptyBlockUntil, End is not require to be empty, so skipEmptyBlockUntil(for.end.loopexit, for.end) would return for.end.

In D94717#2730948, @Whitney wrote:

In D94717#2730582, @bmahjour wrote:

I think this changes the semantics for the cases that we used to handle before. For example wouldn't this require the exit successors to be empty? If so I think lcssa phis can prevent us from detecting guards. We need to test for those cases as well.

LCSSA phis should be in the loop exit block, not exit successors.

LGTM pending indicated test change and no further comments from Michael.

I think this changes the semantics for the cases that we used to handle before. For example wouldn't this require the exit successors to be empty? If so I think lcssa phis can prevent us from detecting guards. We need to test for those cases as well.

LGTM

It was based that function on what Clang generates with the primary intention to generate "your pragma has been ignored" warnings. llvm.loop.unroll.runtime.disable would still allow full unrolling, so it does not fit directly into an enable/disable scheme. Maybe we'd need separate hasFullUnrollTransformation and hasPartialUnrollTransformation functions. Patch welcome.

Changed it to use hasUnrollTransformation() per Michael's comment. Note that hasUnrollTransformation does not take llvm.loop.unroll.runtime.disable into account. I think that's ok for now, since clang doesn't seem to be generating that MD and we only use it for marking epilogue loops. We could add that in the future as a separate patch, if the need arises.

LGTM

Most of the changes in DA results look good to me, except a few I've noted. Perhaps those can be improved in the future, since this patch is making an improvement overall.

The overall approach looks good to me. I might have gone a little too deep into the details with my inline comments, but the direction makes sense to me.

In D100121#2677358, @sdesmalen wrote:

You're right, that comment wasn't correct. I wrote the code last week and incorrectly remembered the change when writing up the comment today.

The issue has to do with the truncation. It stores the floating-point cost in the ProfitableVFs list as unsigned.

For llvm/test/Transforms/LoopVectorize/X86/intrinsiccost.ll, it then compares the costs as follows:

1 < 2 ? true
0 < 1 ? true
0 < 0 ? false

Where it now compares the costs properly as:

5/4 < 5/2 ? true
5/8 < 5/4 ? true
5/16 < 5/8 ? true

The patch isn't entirely NFC because it also fixes an issue in selectEpilogueVectorizationFactor, where the cost passed to ProfitableVFs is no longer the total cost, but rather the cost per lane which is also what is used to determine the VF for the vector body loop.

In D98475#2676997, @congzhe wrote:

I committed and reverted the patch since it fails x86 sanitizer check. The reason is that after the patch, the variable LoopExit becomes unused.

Now I removed LoopExit, I'd appreciate if you could take a look @bmahjour, thanks a lot!

LGTM

Thanks for fixing this problem. I just have some comments about the tests, otherwise LGTM.

clang-format

In D90159#2466599, @MaskRay wrote:

Should this have some tests? Even if guarded by REQUIRES: if some feature is needed.

In D90159#2454905, @bmahjour wrote:

In D90159#2453805, @Meinersbur wrote:

Can I help fixing the Windows build problem?

I think I have a fix (please see the updated patch), but don't have access to a windows machine to verify. Would you be able to try building with MSVC and let me know if it passes?

In D90159#2453805, @Meinersbur wrote:

Can I help fixing the Windows build problem?

fix formatting and use interleaveComma instead of interleave.

LGTM

Thanks for the review @Meinersbur and sorry for taking so long to address your comments.

In D89566#2437988, @nikic wrote:

Should this really be enabled by default given the current (lack of) cost modelling? Primarily concerned about the code size regression this causes.

Sorry, I meant to do a more thorough review before approving. I had a closer look today and found some nits and a couple of questions. Please see inline comments.

OK great. It sounds like there's agreement on the further direction overall and we can work on that in-tree. I'll work on getting the pieces ready to handle live-ins/live-outs in VPlan as required. As for the sekeleton creator, I might be able to take a look over the next week.

LGTM, thanks (meant to respond a bit earlier today :)

Forgot to remove target triple and attributes from target independent tests. They're fixed now.