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.

@fhahn : I noticed that https://bugs.llvm.org/show_bug.cgi?id=31141 stopped crashing with this patch, do you think it's by accident or can that possibly be closed now?

https://bugs.llvm.org/show_bug.cgi?id=41853

When the assertion fails we have:

(gdb) call F.dump()

With this patch the following starts hitting an assertion:

In D60846#1495721, @dlrobertson wrote:

@uabelho great job coming up with a reproducer and thanks @nikic for reverting! I'll start working on a better solution.

I ran ninja check, but I did not hit any failures. Are there more architectures and/or tools I should enable and check before a commit?

This is as far as I've come:

I'm experiencing problems with this patch as well. I'm not 100% sure what the
problem is yet but I have a suspicion:

Anyone has an opinion about this?

Rebased, fixed a spelling mistake.

Should we/someone push this?

Thanks for fixing!
The original case where I saw the crash compiles successfully with this patch.

I see crashed with this patch. E.g.

In D59936#1452754, @hans wrote:

In D59936#1452733, @uabelho wrote:

When looking at some code changes for my out-of-tree target with this commit I noticed a change that I thought I'd ask if you think
is good or not.

With this change we now seem to sink e.g. calls to @llvm.lifetime.end? It returns void, and thus has 0 uses, and as far as I understand
such calls were not sinked before? Is this on purpose and is that good? In general I guess it's good to do lifetime.end at soon as possible?

(And I suppose there are also a whole bunch of other intrinsics that also return void that can be sinked now, I've no idea if that
can cause problems somewhere?)

It's not on purpose in the sense that I was targeting the lifetime intrinsics, but it's showing my change working as intended.

For the lifetime intrinsics, I don't think this really matters. As you say, it's good to do lifetime.end as soon as possible, so sinking them in general doesn't make sense, but I also don't think this matters much here because they can't be sunk very far, i.e. I don't see how this sinking could actually extend the lifetime.

When looking at some code changes for my out-of-tree target with this commit I noticed a change that I thought I'd ask if you think
is good or not.

I noticed that with this commit I get a whole bunch (~40) of warnings like the below when compiling with gcc 7.4:

[10/16] Building CXX object tools/clang/tools/extra/clang-tidy/utils/CMakeFiles/clangTidyUtils.dir/HeaderGuard.cpp.o
In file included from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.h:12:0,
                 from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.cpp:9:
../tools/clang/tools/extra/clang-tidy/utils/../ClangTidy.h:161:16: warning: 'virtual void clang::tidy::ClangTidyCheck::registerPPCallbacks(const clang::SourceManager&, clang::Preprocessor*, clang::Preprocessor*)' was hidden [-Woverloaded-virtual]
   virtual void registerPPCallbacks(const SourceManager &SM, Preprocessor *PP,
                ^~~~~~~~~~~~~~~~~~~
In file included from ../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.cpp:9:0:
../tools/clang/tools/extra/clang-tidy/utils/HeaderGuard.h:35:8: warning:   by 'virtual void clang::tidy::utils::HeaderGuardCheck::registerPPCallbacks(clang::CompilerInstance&)' [-Woverloaded-virtual]
   void registerPPCallbacks(CompilerInstance &Compiler) override;
        ^~~~~~~~~~~~~~~~~~~