In D72734#1822172, @fpetrogalli wrote:

If you are happy with it, may I ask you to formally approve it so I can submit it? (Feel free to do a full review if you prefer to do so!)

In my out-of-tree target we are running DeadMachineInstructionElim after SSA, and now of course the new assert fails.

In D72734#1820557, @fpetrogalli wrote:

@uabelho , let me know if the introduction of isTLIScalarize prevents the regressions you have seen in your out-of-tree compiler.

In D67572#1817461, @fpetrogalli wrote:

Would that work for you?

I'll submit this for @ebevhan in a little bit.

Rebased, updated to drop dead flags as well.

In D67572#1781924, @fpetrogalli wrote:

Ah, OK. I see what you mean. I think we could solve this by scalarizing after vectorization, instead of doing it in the vectorizer.

We could have a IR pass that runs after the vectorizer that looks for intrinsics calls .When it sees an intrinsics that operates on vector, it checks whether the target is able to lower it to some function or instruction. If not, it scalarizes it. With this we wouldn't have to introduce special behavior in the vectorizer for handling intrinsics: it could just vectorize any call to intrinsics for which vectorization make sense.

@uabelho: would it be possible for you to provide me a minimal reproducer that I could use to craft the (wip) solution I have in mind?

In D67572#1781363, @fhahn wrote:

[...]

Is this change in behavior intended?

This change in behavior sounds a bit worrying for down-stream targets.

I think we should have at least an assertion making sure the check in LoopVEctorizationLegality succeeds in all cases it did previously with isFunctionVectorizable, otherwise down-stream targets will silently miss out on vectorization.

Thanks for the reply!

A question regarding this patch.
For my out-of-tree target vectorization of intrinsics added for my target seems to have stopped working with this patch.
Is there something/what do I have to do to make the vectorizer understand my intrinsics are vectorizable?

I think you're on the right track. My mir-reproducer passes with this patch, so now I get the wanted

In D70676#1777672, @jmorse wrote:

D71283 Should (TM) fix it, but, the x86 test that I wrote and which seemed to replicate the issue, now no longer replicates the issue. Which is unfortunate.

Would you be able to give that patch (and its parent) a test to see whether it fixes the problem you're encountering? Storing the subregister separately should entirely fix this I believe, I'm just having problems replicating it on x86.

I think you're on the right track. My mir-reproducer passes with this patch, so now I get the wanted

In D70942#1776042, @efriedma wrote:

The load version is a lot more instructions, so it's probably not worth it... the current patch should be fine as-is.

In D70942#1773282, @efriedma wrote:

LGTM. (Left some notes on the testcase if anyone else wants to follow.)

In D70676#1775047, @jmorse wrote:

Ouch, yeah, I can see exactly why that is: the sunk DBG_VALUE is copied from the old / "original" DBG_VALUE, which in the meantime has been copy-propagated. And through the copy propagation, it grows an extra piece of subregister information.

This should be straightforwards to fix, I'll have a patch up shortly, thanks for the report!

I've started seeing a verifier complaint with this patch.
I don't have a full testcase to share since I get this on our out-of-tree
target but maybe you can make somethng out of this anyway.

Thanks for the suggestion @spatel. Updated the testcase.

Added pre-commit of the testcase, changed testcase to run all the way to ASM output so we can use utils/update_llc_test_checks.py.

Originally triggered this problem for our out-of-tree target but I think it's exposed on ARM as well.

In D70298#1747644, @uabelho wrote:

Nice! This indeed seems to solve the problem we saw in PR40816.

I don't know the code but I can at least provide some additional testing by including this patch in my weekend testing to see nothing unexpected pops up there.

Nice! This indeed seems to solve the problem we saw in PR40816.

In D69935#1739235, @dblaikie wrote:

In D69935#1738273, @uabelho wrote:

Don't you need to also remove

case LinkageSpecDecl::lang_cxx_11:
case LinkageSpecDecl::lang_cxx_14:

from VisitLinkageSpecDecl in clang-tools-extra/modularize/Modularize.cpp? (added in r372714 / e07376a320d to silence a clang warning).
I can't see how that code would compile otherwise?

+1 to this

Don't you need to also remove

In D69028#1712384, @uabelho wrote:

I got another testcase that also started failing with the original commit that I hoped would be the same problem, but that isn't solved yet with this fix so I'll need to dig into that one and see what's going on there.

That was probably false alarm, I think it's a problem in our testcase rather than in the patch.

I got another testcase that also started failing with the original commit that I hoped would be the same problem, but that isn't solved yet with this fix so I'll need to dig into that one and see what's going on there.

In D69028#1711048, @jmorse wrote:

Cool, here's a version with deref_size;

In D68945#1710698, @jmorse wrote:

Hi Mikael,

In D68945#1710654, @uabelho wrote:

And before this change we got the following after Isel:

DBG_VALUE %fixed-stack.1, 0, !"bar_y", !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_stack_value)

With this change, we instead get this after Isel:

DBG_VALUE %fixed-stack.1, $noreg, !"bar_y", !DIExpression(DW_OP_LLVM_convert, 16, DW_ATE_signed, DW_OP_LLVM_convert, 32, DW_ATE_signed, DW_OP_deref, DW_OP_stack_value)

Isn't the DW_OP_deref inserted in the wrong place? I think it should be done before the DW_OP_LLVM_convert:s rather than after?

You're right, that definitely looks wrong. This change was only supposed to affect DBG_VALUEs generated from indirect intrinsics (dbg.addr/dbg.declare); at a guess, it looks like dbg.values of Values that becomes FrameIndexes have the IsIndirect flag set too. I'll try to replicate this locally and patch it, please do back the commit out if it's causing significant misery.

I'm struggling with a case for my out-of-tree target where I think debug info is wrong with this change.

In D68460#1695283, @kuhar wrote:

Would it be possible to add a verifyAnalysis function to MLI and check if a freshly calculated one matches the 'preserved' one?

clang-cuda-build buildbot failed too:
http://lab.llvm.org:8011/builders/clang-cuda-build/builds/37865/steps/ninja%20check%201/logs/stdio

Does this make sense?

In D67832#1683474, @efriedma wrote:

LGTM

In D67832#1681258, @efriedma wrote:

Would it make more sense to just say this isn't a triangle? We don't really want to do loop peeling in IfConversion.

Disallow this case in ValidTriangle instead.

In D67613#1680176, @uabelho wrote:

In D67613#1679879, @xbolva00 wrote:

/srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13: warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Wswitch]
1 warning generated.

We also get that warning when compiling with clang 8:

../../clang-tools-extra/modularize/Modularize.cpp:583:13: error: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Werror,-Wswitch]
    switch (D->getLanguage()) {
            ^
1 error generated.

In D67613#1679879, @xbolva00 wrote:

/srv/llvm-buildbot-srcatch/llvm-build-dir/clang-x86_64-debian-fast/llvm.src/tools/clang/tools/extra/modularize/Modularize.cpp:583:13: warning: enumeration values 'lang_cxx_11' and 'lang_cxx_14' not handled in switch [-Wswitch]
1 warning generated.

It fixes the problem I reported. Thanks!

I've no idea if the fix itself makes sense but at least it does seem to fix my reproducer for PR38513 and I didn't see any problems with the patch during the (limited) testing I've done.

In D67171#1664690, @davezarzycki wrote:

Hi @uabelho – Try r371521

When compiling with gcc, we get warnings with this commit:

[100/4153] Building C object lib/Support/CMakeFiles/LLVMSupport.dir/regfree.c.o
cc1: warning: command line option '-fno-rtti' is valid for C++/ObjC++ but not for C
[101/4153] Building C object lib/Support/CMakeFiles/LLVMSupport.dir/regstrlcpy.c.o
cc1: warning: command line option '-fno-rtti' is valid for C++/ObjC++ but not for C
[102/4153] Building C object lib/Support/CMakeFiles/LLVMSupport.dir/regerror.c.o
cc1: warning: command line option '-fno-rtti' is valid for C++/ObjC++ but not for C
[107/4153] Building C object lib/Support/CMakeFiles/LLVMSupport.dir/regexec.c.o
cc1: warning: command line option '-fno-rtti' is valid for C++/ObjC++ but not for C
[114/4153] Building C object lib/Support/CMakeFiles/LLVMSupport.dir/regcomp.c.o
cc1: warning: command line option '-fno-rtti' is valid for C++/ObjC++ but not for C

I did a minor change to the test case in r371171 since the

;CHECK: bb

checks matched on the line

;ModuleID = '/repo/bb/

in the output instead of the wanted "bb.0:".

Normally we do use basicaa, the crash was found in "fuzz" tests where we compile the code with random flags.

Why would it be ok to give the assert when we run

In D66803#1656923, @uabelho wrote:

Hi,

I've hit the new assertion when compiling for my out of tree target when using -Osize in combination with

#pragma clang loop vectorize(enable)

on a loop.

What is supposed to prevent us from triggering the assertion in a case like that?

I've hit the new assertion when compiling for my out of tree target when using -Osize in combination with

In D62644#1543279, @fhahn wrote:

It looks like this patch may have caused https://bugs.chromium.org/p/oss-fuzz/issues/detail?id=15236. It would be great if you could have a look.

With r363169 the problem seems to be fixed, so this patch is probably not needed anymore.