We've run into a case where instcombine crashes with this patch:

opt -o /dev/null bbi-47401.ll -instcombine

on

@c = external global i16*, align 1

In D87163#2270918, @uabelho wrote:

In D87163#2270860, @fhahn wrote:

In D87163#2270568, @uabelho wrote:

Hi,

I'm seeing what I think is a miscompile with this:

Thanks for the reproducer. We have to be more careful around pointers that may reference multiple locations in memory. I pushed f715d81c9df3 which limits elimination to stores that only reference a single location (or are in the same BB). The check is conservative and can be improved in the future, but the impact on the number of stores eliminated seems very low in practice. It would be great if you could check if that fixes the original reproducer.

Great! I'll check and get back during the day.

In D87163#2270860, @fhahn wrote:

In D87163#2270568, @uabelho wrote:

Hi,

I'm seeing what I think is a miscompile with this:

Thanks for the reproducer. We have to be more careful around pointers that may reference multiple locations in memory. I pushed f715d81c9df3 which limits elimination to stores that only reference a single location (or are in the same BB). The check is conservative and can be improved in the future, but the impact on the number of stores eliminated seems very low in practice. It would be great if you could check if that fixes the original reproducer.

I wrote https://bugs.llvm.org/show_bug.cgi?id=47517 about a crash that starts happening with this patch.

I'm seeing what I think is a miscompile with this:

opt -S -dse -o - dse.ll

on

The following crashes for me with this patch
opt -memoryssa -gvn -early-cse-memssa -S -o - foo.ll

We ran into problems with this commit.

I wrote
https://bugs.llvm.org/show_bug.cgi?id=47381
about a similar problem also caused by the missing setting of Changed in SimplifyIndvar::eliminateOverflowIntrinsic.

We ran into a problem with this patch. I wrote https://bugs.llvm.org/show_bug.cgi?id=47343 about it.

We've run with changes like this for a long time to make the routines work for our out of tree target where int is also 16 bits so thumbs up from me even if I'm not sure I can formally LGTM it.

In D82998#2143456, @dmgreen wrote:

Thanks. I will take a look.

From this, that bug appears to be fixed: https://godbolt.org/z/6W8hc8. Probably from D82987 I would expect. I will check. Opt 10.0.0 does show the problem, where as trunk seems to be fixed.

Let me know about the runtime failure.

I noticed that we got a runtime failure with this patch for our out-of-tree target. I haven't gone to the bottom with it yet, but I saw that a colleague of mine wrote a PR about a miscompile with -basic-aa-recphi that at least hasn't been handled according to bugzilla:
https://bugs.llvm.org/show_bug.cgi?id=37952

I start seeing a crash with this patch:
https://bugs.llvm.org/show_bug.cgi?id=46652

A whole bunch of buildbots seems to have failed as well (in different ways).
E.g.
http://lab.llvm.org:8011/builders/llvm-clang-win-x-armv7l/builds/227/steps/test-libc%2B%2B/logs/FAIL%3A%20libc%2B%2B%3A%3Aunique_ptr_arg.pass.cpp
hit the same assertion as I see.
How about reverting again?

I hit this assertion in a couple of testcases when running check-all on trunk now:

I don't know this code but at least the crash I saw goes away with the fix. Thanks!

I wrote
https://bugs.llvm.org/show_bug.cgi?id=46430
about a crash that started happening with this patch.

In D81920#2109901, @kadircet wrote:

Can you check if you see the warning with different versions of gcc?

In D82029#2109167, @lxfind wrote:

Test failures are being fixed in https://reviews.llvm.org/D82338/new/

I see a bunch of failures when I run libcxx testcases with this patch. Should there be "noexcept" on a number of more places?

I wrote
https://bugs.llvm.org/show_bug.cgi?id=46343
about a crash that starts happening with this patch.

In D81634#2087298, @spatel wrote:

Was the failure only a fuzzer problem?

In D79218#2088038, @rampitec wrote:

In D79218#2087450, @uabelho wrote:

Hi!

I wrote
https://bugs.llvm.org/show_bug.cgi?id=46280
for a crash that started happening with this commit.

Thanks! Here is the fix: D81674

I wrote
https://bugs.llvm.org/show_bug.cgi?id=46280
for a crash that started happening with this commit.

Thanks for the patch so quickly!
I really have no idea if the assert actually makes sense or not though, so it's best if @spatel can have an opinion.

I wrote a PR about a crash that starts occuring with this patch:
https://bugs.llvm.org/show_bug.cgi?id=46276

I just wrote a PR about a debug-info fault that starts occuring with this patch:
https://bugs.llvm.org/show_bug.cgi?id=46267

Our out-of-tree target also have 16 bit ints and we've had to change a whole bunch of builtin_clz to clzsi and builtin_ctz to ctzsi to make things work so this patch is a step in the right direction for us too.

With gcc 7.4, the following warning pops up with this patch:

In D80863#2070105, @kcc wrote:

Hi, 
This made our ubsan bots red. Please fix or revert ASAP
http://lab.llvm.org:8011/builders/sanitizer-x86_64-linux-fast/builds/42256

Hi!
PR 46098 started occuring wiith this patch:
https://bugs.llvm.org/show_bug.cgi?id=46098

In D79467#2035176, @rnk wrote:

Hopefully rG409274274 addresses the GCC warnings. I didn't get a chance to locally repro and confirm that the fix works.

I wrote https://bugs.llvm.org/show_bug.cgi?id=45885 about a crash which starts happening with this patch.

Great! Thanks!

It seems to be this change in SemaStmt.cpp that makes the FENV-pragma have some effect regardless of the warning saying that the pragma doesn't have any effect:

I think I'm seeing some oddities with this patch (current trunk, 0054c46095).
With

In D77620#1985961, @asbirlea wrote:

While this solution does solve the problem, there's an underlying bug that this is hiding. I've talking with Chandler about this and I'll send a couple of patches that are aiming to fix the underlying problem instead.
Apologies for the delay in getting you a fix.

I've run quite a number of tests with this without problems.

I wrote
https://bugs.llvm.org/show_bug.cgi?id=45525
for a crash that started appearing with this patch.

Thanks!
Submitted in 893df2032d4.

I have no idea if this is the best fix for
https://bugs.llvm.org/show_bug.cgi?id=45302
but I saw that we've made a similar fix for triangles in 957e090ac9 and thought maybe this could be an easy way out in this case.

I don't really know this code but at least the fix avoids the crash I saw. Thanks!

Thanks! This indeed seem to solve the problem I saw.

@nikic Alright, thanks!

Is this supposed to affect the output from instcombine?

I have no idea if it's reasonable to remove these methods but it does indeed silence the compiler warnings about unused functions that I saw. Thanks for that.

In D51028#1839423, @jonpa wrote:

I am a little curious as to how these differences in flags arise. I can understand that a kill flag can be seen as an optimization and is optional. The same would go for a dead flag. But I am not sure about an undef flag... If it is present in one case but not in the other, the two instructions do not seem equivalent, or?

So I suppose your patch makes sense to me, although I am not familiar enough to understand the undef case... Perhaps some comment on how these three cases can emerge and may be treated would be good? (at least to me :-)

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