[PATCH] D65336: [InstSimplify] remove quadratic time looping (PR42771)

[Bjorn Pettersson via Phabricator via llvm-commits]

bjope added inline comments.


================
Comment at: llvm/trunk/lib/Transforms/Scalar/InstSimplifyPass.cpp:47
+        if (isInstructionTriviallyDead(&I)) {
+          DeadInstsInBB.push_back(&I);
+        } else if (!I.use_empty()) {
----------------
I think we need to set `Changed = true` here as well.

I've got a test case like this:


```
; NOTE: Assertions have been autogenerated by utils/update_test_checks.py
; RUN: opt < %s -instsimplify -S -o /dev/null -debug-pass=Details 2>&1 | FileCheck --check-prefix DETAILS %s
; RUN: opt < %s -instsimplify -S -o - | FileCheck %s

; Verify that InstSimplifyLegacyPass notifies the pass manager about changes
; being made (when a call is removed CGSCC must be updated).
;
; DETAILS: Made Modification 'Remove redundant instructions' on Function 'main'

define internal void @func_1(i64* nocapture readnone %0) #0 {
; CHECK-LABEL: @func_1(
; CHECK-NEXT:    unreachable
;
  unreachable
}

define i16 @main(i16 %0, i16** nocapture readnone %1) #1 {
; CHECK-LABEL: @main(
; CHECK-NEXT:  bb1:
; CHECK-NEXT:    unreachable
;
bb1:
  call void @func_1(i64* undef)
  unreachable
}

attributes #0 = { noinline norecurse nounwind readnone }
attributes #1 = { norecurse nounwind readnone }
```

that show that we do not propagate to pass managers that we have removed a call otherwise.

My original reproducer ended up with

  opt: ../include/llvm/Analysis/CallGraph.h:180: llvm::CallGraphNode::~CallGraphNode(): Assertion `NumReferences == 0 && "Node deleted while references remain"' failed.

due to not cleaning up the call graph after removing the call.

I'll upload a patch for this (assuming it is the correct solution).

Although, I've also noticed that the same assert was seen in https://bugs.llvm.org/show_bug.cgi?id=42751, and https://bugs.llvm.org/show_bug.cgi?id=42787 is also related to calls being optimized away (afaict). InstSimpllify is not involved in those two PRs. So it might be something else that makes isInstructionTriviallyDead returning that certain calls can be removed nowadays (such as recent changes regarding functions attributes). It should be valid for a FunctionPass to delete a call right, or is it interprocedural to for example look at function attributes to determine that the callee has no side effects? It is illegal for a FunctionPass to inspect a function other than the one being optimized. Is looking at function attributes the same as "inspecting" the callee?




Repository:
  rL LLVM

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

https://reviews.llvm.org/D65336