[PATCH] D152691: [LICM] Sunk instructions with invalid source location.

[Felipe de Azevedo Piovezan via Phabricator via llvm-commits]

fdeazeve added a comment.

I think we can greatly simplify the test to make it easier for readers to understand what's going on. I left some inline comments, and the test itself could start along these lines:

  ; CHECK-LABEL: for.end:
  ; CHECK: tail call noundef i32 @foo0({{.*}}), !dbg ![[DBG:[0-9]+]]
  ; CHECK-DAG: ![[DBG]] = !DILocation(line: 0, scope: ![[SCOPE:[0-9]+]]
  ; CHECK-DAG: ![[SCOPE]] = distinct !DISubprogram(name: "bounce",
  
  define noundef i32 @foo0(i32 noundef %Idx) #0 !dbg !9 {
    ret i32 2, !dbg !14
  }
  define noundef i32 @foo1(i32 noundef %Idx) #0 !dbg !15 {
    ret i32 4, !dbg !16
  }
  
  define i32 @foo2() !dbg !17 {
  entry:
    br label %for.body, !dbg !20
  
  ... rest of the test here ...



================
Comment at: llvm/test/Transforms/LICM/sink-instruction-invalid-location.ll:30
+; CHECK-LABEL: for.end:
+; CHECK: %call1.le = tail call noundef i32 @"?getInOrder@@YAHH at Z"(i32 noundef %I.07.lcssa), !dbg ![[DBG:[0-9]+]]
+; CHECK-DAG: ![[DBG]] = !DILocation(line: 0, scope: ![[SCOPE:[0-9]+]]
----------------
We shouldn't hardcode the name of SSA values, these are not guaranteed to be preserved by opt.

I believe the same apply for the names of BBs (which are also SSA values)


================
Comment at: llvm/test/Transforms/LICM/sink-instruction-invalid-location.ll:35
+@"?InOrder@?1??getInOrder@@YAHH at Z@4PAHA" = internal unnamed_addr constant [2 x i32] [i32 0, i32 1], align 4
+@"?RandVars@?1??getRandVar@@YAHH at Z@4PAHA" = internal unnamed_addr constant [2 x i32] [i32 4, i32 4], align 4
+
----------------
I believe this variable is not used anywhere?


================
Comment at: llvm/test/Transforms/LICM/sink-instruction-invalid-location.ll:39
+define dso_local noundef i32 @"?getInOrder@@YAHH at Z"(i32 noundef %Idx) local_unnamed_addr #0 !dbg !9 {
+entry:
+  %idxprom = sext i32 %Idx to i64, !dbg !14
----------------
If I understand correctly, the contents of this function are not needed by the test and could be replaced by a `ret i32 2`

Note this will also make the other GV unused.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D152691