In D95217#2515072, @aqjune wrote:

That was my epic typo, sorry. It should have been select i1 false, i1 poison, i1 false because the buggy case was encoding the path that visits cond.false.

Great, then I'm slightly less puzzled :) Thanks!

Thanks for fixing this!
I don't know this code at all, but I'm puzzled by this sentence in the summary:

The path for.body -> cond.false -> cond.end should be taken when 'select i1 true, i1 poison, i1 false' holds

I wrote a PR about this:
https://bugs.llvm.org/show_bug.cgi?id=48832

I've been able to reproduce the problem I see for an in-tree architecture.
So with

opt -mtriple=s390x-unknown-linux -mcpu=z13 -loop-vectorize -S -o - bbi-51947.ll

I get

store <4 x i32> poison, <4 x i32>* %6, align 1

but before this patch I got

store <4 x i32> zeroinitializer, <4 x i32>* %6, align 1

The input is

%arrayt = type [64 x i32]

I'm not sure what happens yet, but with this patch I'm seeing a miscompile.
Diffs start occuring after the loop vectorizer. Previously we got this store

store <4 x i32> zeroinitializer, <4 x i32>* %8, align 1, !dbg !16

but now we instead get this:

store <4 x i32> poison, <4 x i32>* %8, align 1, !dbg !16

Are you aware of any problems?

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

In rG2d07414ee5f74a09fb89723b4a9bb0818bdc2e18#968318, @lebedev.ri wrote:

Fixed by 29ca7d5a1ad968c371124b8d82edd8d91eee7b4f, thanks!

I see a failed assertion with this patch if I run

opt -o /dev/null bbi-51466.ll -simplifycfg

on the input

I pushed this for @ehjogab

In rGb21840751278128ef6942074ae89ea63c9c6ac58#967713, @bjope wrote:

In rGb21840751278128ef6942074ae89ea63c9c6ac58#967637, @uabelho wrote:

Hi @nikic,

I've seen a miscompile for my OOT target with this patch. Unfortunately I haven't gone to the bottom with it yet (and I don't think I will until after new years), so I'm not sure what the problem is exactly, but I think that AA comes to the conclusion that a read and a write to the exact same location doesn't alias, and then things go wrong. Are you aware of any problems?

@uableho: Our problem seem to be related to a bad DominatorTree (exposed by this patch). If I do

@@ -2178,8 +2186,14 @@ static bool isKnownNonNullFromDominatingCondition(const Value *V,
           BasicBlock *NonNullSuccessor =
               BI->getSuccessor(NonNullIfTrue ? 0 : 1);
           BasicBlockEdge Edge(BI->getParent(), NonNullSuccessor);
-          if (Edge.isSingleEdge() && DT->dominates(Edge, CtxI->getParent()))
+          DominatorTree DT2(*NonNullSuccessor->getParent());
+          if (Edge.isSingleEdge() && DT2.dominates(Edge, CtxI->getParent())) {

to force a recalculation of DT then the miscompile is gone. Ouch, that could be a much bigger problem than I expected here. I haven't had time to figure out why the DT is incorrect (I assume that since this is deep down in the backend when doing misched the DomTree (on IR level) should just be preserved by the PM, or recalculated when swapping functions, but I don't really know the details).

I've seen a miscompile for my OOT target with this patch. Unfortunately I haven't gone to the bottom with it yet (and I don't think I will until after new years), so I'm not sure what the problem is exactly, but I think that AA comes to the conclusion that a read and a write to the exact same location doesn't alias, and then things go wrong. Are you aware of any problems?

In rGd4ccef38d0bbcd70f56d586b4dfc988db863e388#967487, @lebedev.ri wrote:

Thank you for the reproducer!

In rGd4ccef38d0bbcd70f56d586b4dfc988db863e388#967474, @uabelho wrote:

In rGd4ccef38d0bbcd70f56d586b4dfc988db863e388#967468, @lebedev.ri wrote:

In rGd4ccef38d0bbcd70f56d586b4dfc988db863e388#967466, @uabelho wrote:

Hi,

I'm still

Hm, have you previously already posted the original reproducer and i missed it?

Oh, no I haven't but I saw someone else reporting a hang a think, and saw this fix and then just noticed this fix didn't solve my hang.
Sorry for the confusion.

Np. Fixed in 374ef57f1379d3d3bbe5bfb19f1d2ea7e79b6db9.

In rGd4ccef38d0bbcd70f56d586b4dfc988db863e388#967468, @lebedev.ri wrote:

In rGd4ccef38d0bbcd70f56d586b4dfc988db863e388#967466, @uabelho wrote:

Hi,

I'm still

Hm, have you previously already posted the original reproducer and i missed it?

I'm still seeing a case where instcombine hangs after d9ebaeeb468 "[InstCombine] Hoist xor-by-constant from xor-by-value", even with this fix.

In D92726#2472881, @nikic wrote:

@uabelho Thanks for the reproducer! This issue should be fixed by https://github.com/llvm/llvm-project/commit/dcd21572f971ae5b5f1bf1f1abefafa0085404e1.

The following starts failing with this commit:

opt -S -o /dev/null bbi-51071.ll -early-cse

With bbi-51071.ll being

@f.a = external global i16, align 1

I submitted this for @ehjogab

I submitted this for @ehjogab

In D82005#2446532, @spatel wrote:

This should handle the test shown: 4f051fe

We've noticed a crash that started occuring with this patch.
Running

In D92270#2422741, @aqjune wrote:

In D92270#2422661, @uabelho wrote:

Should langref also be updated to describe this? Or does it already?
I just found this
"An instruction that depends on a poison value, produces a poison value itself."
here
http://llvm.org/docs/LangRef.html#poison-values

I think "Values other than phi nodes and select instructions depend on their operands." was supposed to say that. :)

In D92270#2422625, @aqjune wrote:

Hi,

It seems it is related to two optimizations:
(1) select i1 x, true, y -> or i1 x, y
(2) or i1 x, poison -> poison

Semantically, the first one is broken. It needs to freeze y. But, it will introduce a lot of freeze instructions. The clang patches that introduce noundef to function arguments is still ongoing.
Another workaround is to weaken (2) to or i1 x, poison -> x. This fixes the miscompilation; I'll push a commit that does this.

I'm seeing a miscompile with this patch. I'm not very good at the semantic differences between undef and poison so I don't know really where it goes wrong.

Another little ping.

I submitted this for @ehjogab

In D90610#2405687, @spatel wrote:

LGTM

In D90610#2376116, @mtrofin wrote:

In D90610#2370539, @uabelho wrote:

In D90610#2368489, @mtrofin wrote:

Thanks for looking into this!

Should places where we use ConstantOffsetPtrs to determine if instructions may be simplified, like CallAnalyzer::visitCmpInst, be also adjusted? Or does the ContantExpr::getICmp call take care of different-sized constant ints?

I don't know. When I debugged PR49890 I just realized that the size mismatch was introduced with the ptrtoint, so I tried doing something about that instead of trying to handle it at every use of ConstantOffsetPtrs. And then I realized that fix seems to have fixed PR38500 as well.

I think it would be good to understand that, as part of this patch, as changing that may have performance implications.

Do the easiest fix.

In D91383#2403369, @nikic wrote:

@uabelho Thanks for the report! I have reverted the assert in https://github.com/llvm/llvm-project/commit/85ccdcaa502ee2c478f2d0ba2b1e217117b69032. We'll have to fix this asymmetric in phi-phi aliasing before re-enabling it.

The following hits an assertion with this patch. If I run

opt -S -o - aa.ll -aa-eval

Thanks!

Hi!
This seems to fail if one of the predecessors to the PHI block is dead and contain weird stuff e.g. like this:

define void @foo() {
bb.0:
  br label %bb.1

Thanks! The problem I saw is fixed with this patch.

I hit an assertion with this patch:
opt: ../lib/IR/Type.cpp:620: static llvm::FixedVectorType *llvm::FixedVectorType::get(llvm::Type *, unsigned int): Assertion `isValidElementType(ElementType) && "Element type of a VectorType must " "be an integer, floating point, or " "pointer type."' failed.

In D87163#2373580, @uabelho wrote:

Hi!

I found a new problem after this was turned on again in 51ff04567b2f.

I found a new problem after this was turned on again in 51ff04567b2f.
With

opt -S -o - bbi-49235.ll -memoryssa -gvn -dse -verify-memoryssa

I hit

opt: ../lib/Analysis/MemorySSA.cpp:2063: void llvm::MemorySSA::verifyOrderingDominationAndDefUses(llvm::Function &) const: Assertion `&*ALI == *AAI && "Not the same accesses in the same order"' failed.

with bbi-49235.ll being

define void @k() {
entry:
  %tobool = icmp ne i16 1, 0
  %0 = xor i1 %tobool, true
  call void @llvm.assume(i1 %0)
  call void @f()
  ret void
}

Bah, I meant to post my comment in https://reviews.llvm.org/D89613. I did now.

Is this commit supposed to change the output from llc?

Is this commit supposed to change the output from llc?

In D90610#2368489, @mtrofin wrote:

Thanks for looking into this!

Should places where we use ConstantOffsetPtrs to determine if instructions may be simplified, like CallAnalyzer::visitCmpInst, be also adjusted? Or does the ContantExpr::getICmp call take care of different-sized constant ints?

Changed names of the testcase functions and changed to zext in visitPtrToInt.

We got similar problems for our downstream target. We worked around that by bailing out early in PragmaSTDC_FENV_ACCESSHandler::HandlePragma for our target but perhaps one could check hasStrictFP() instead and ignore the pragma if that is false?

Are there any outstanding issues left or is this perhaps good enough to be submitted?

I don't know what the best fix is, but I've verified that this patch solves the problem I saw and I haven't seen anything weird pop up with the patch in some limited testing I've done with it.
Thanks!

https://bugs.llvm.org/show_bug.cgi?id=47945 started happening with this patch.

In D88806#2327012, @lebedev.ri wrote:

Perfect, thank you. That's the test case i was missing. Will fix in a moment.

Fixed in rG7324616660fc0995fa8c166e3c392361222d5dbc, thank you for the reproducer!

I got my share of doom too, even after the fix.
So

opt -S -o - bbi-48445.ll -indvars

crashes with

opt: ../lib/IR/Constants.cpp:1896: static llvm::Constant *llvm::ConstantExpr::getZExt(llvm::Constant *, llvm::Type *, bool): Assertion `C->getType()->isIntOrIntVectorTy() && "ZEXt operand must be integral"' failed.

for the following input:

I just wrote
https://bugs.llvm.org/show_bug.cgi?id=47753
about a failure that starts occuring with this patch.

Thanks!

I submitted for ehjogab.

Hi!
I don't understand why, but with this commit I start seeing several warnings like this when compiling builtins:

I submited this for ehjogab

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.