[PATCH] D116895: Fix a missed opportunity to optimize consecutive stores.

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added a comment.

In D116895#3269548 <https://reviews.llvm.org/D116895#3269548>, @uabelho wrote:

> Hi,
>
> Anyone seen problems with this patch?
> I have a case for my out-of-tree target where this patch causes two stores to be merged leading to a cycle in the DAG.
> So in the initial DAG we have the following dependencies
>
>   store2 <- load1 <- load2 <- load3 <- store1
>
> and then store2 and store1 are merged to store21 and we end up with dependencies like this:
>
>   store12 <- load1 <- load2 <- load3
>                             <- store12
>
> ie. load2 depends on both load3 and store12 so we have a broken DAG with a cycle in it.
> So then we end up with
>
>   Operand not processed?0x882a240
>   t9: i16,ch = load<(load (s16) from %ir.next1.i)> t26, t8, undef:i16
>   
>   UNREACHABLE executed at ../lib/CodeGen/SelectionDAG/LegalizeTypes.cpp:503!
>
> I haven't been able to reproduce this for any in-tree targets yet so it might very well be something we've broken for our target but I thought I'd mention here that we see problems in case anyone else does too.

Here is a reproducer using aarch64 as target:

  ; RUN: llc -mtriple aarch64-- -o /dev/null %s | FileCheck %s
  
  %str0 = type { i64, i64 }
  %str1 = type { i64, %str1* }
  
  @g0 = external global %str0, align 1
  
  define i64 @foo() {
  entry:
    %sp0 = getelementptr inbounds %str0, %str0* @g0, i32 0, i32 0
    %sp1 = getelementptr inbounds %str0, %str0* @g0, i32 0, i32 1
    store i64 0, i64* %sp1, align 1, !tbaa !1
    %l0 = load %str1*, %str1** undef, align 1
    %lp0 = getelementptr inbounds %str1, %str1* %l0, i32 0, i32 1
    %l1 = load %str1*, %str1** %lp0, align 1
    %lp1 = getelementptr inbounds %str1, %str1* %l1, i32 0, i32 0
    %l2 = load i64, i64* %lp1, align 1, !tbaa !7
    store i64 0, i64* %sp0, align 1
    ret i64 %l2
  }
  
  !llvm.ident = !{!0}
  
  !0 = !{!"clang version 14.0.0.prerel"}
  !1 = !{!2, !6, i16 1}
  !2 = !{!"dinges", !3, i16 0, !6, i16 1}
  !3 = !{!"int", !4, i16 0}
  !4 = !{!"omnipotent char", !5, i16 0}
  !5 = !{!"Simple C/C++ TBAA"}
  !6 = !{!"any pointer", !4, i16 0}
  !7 = !{!2, !3, i16 0}

As mentioned here https://discourse.llvm.org/t/dagcombiner-chains-and-mergeconsecutivestores/59623 , maybe the fault isn't exactly related to this patch (even if it was exposed by this patch) but rather that `checkMergeStoreCandidatesForDependencies` need to analyse the chain operands as well.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D116895/new/

https://reviews.llvm.org/D116895