[PATCH] D78614: [AssumeBuilder] Add assume-simplify pass

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

This is cool, thanks! We might soon have all we need :)

Do we have a test with argument attributes that make assumptions obsolete?



================
Comment at: llvm/lib/Analysis/AssumeBundleQueries.cpp:137
     return RetainedKnowledge::none();
-  auto *Intr = cast<IntrinsicInst>(U->getUser());
   RetainedKnowledge RK =
----------------
Commit this as nfc or merge it into the commit that made it unsused or use Intr below.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:232
+      if (!V)
+        continue;
+      IntrinsicInst *Assume = cast<IntrinsicInst>(V);
----------------
Why `Value*` and how can it be `null`? 


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:250
+        }
+      delete Assume;
+    }
----------------
Can we user eraseFromParent here instead of the two step process?

Think about early continue in the loop (Begin == End).

The `(!I)` can be folded into the next condition.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:265
+        if (BOI.Tag == IgnoreTag)
+          CleanupToDo.insert(Assume);
+        RetainedKnowledge RK = getKnowledgeFromBundle(*Assume, BOI);
----------------
There is no knowledge in ignore tags, right? I guess we could add a continue to not rely on the one 3 lines below.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:276
+                    Assume, &*F.getEntryBlock().getFirstInsertionPt()))
+              goto update;
+          }
----------------
What is updated here? I first thought the argument attribute is merged into the assume but I cannot figure out how the value is passed. I guess a lambda makes this clear (see also below).

On second thought, I doubt we can update the llvm.assume, so if this is what you are doing, I doubt it is legal. It works for "bit-wise" attribues like `nonnull` but not for "stateful" ones like `dereferenceable`.

Thinking this further, we need to distinguish these "stateful" attributes in other places as well. The attributor performs a similar lookup (mistake) right now (I think).


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:286
+            else if (isValidAssumeForContext(Assume, Elem.first, DT))
+              goto update;
+          }
----------------
Same as with the update above.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:300
+        }
+        BOI.Tag = IgnoreTag;
+      }
----------------
Can we make these lambdas?
```
-goto remove
+{ Remove(BOI); continue; }
```

---

`Arg` is probably not the best name for the Use* above.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:315
+
+    Instruction *InsertPt = IsFirst ? BB->getFirstNonPHI() : *Begin;
+    for (IntrinsicInst *I : make_range(Begin, End)) {
----------------
Why can we move assumptions up to the first PHI?


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:329
+      }
+    }
+    if (InsertPt->comesBefore(*Begin))
----------------
What happens to the conditions (operand 0)? 


================
Comment at: llvm/lib/Transforms/Utils/AssumeBundleBuilder.cpp:380
+  }
+};
+
----------------
We need comments on these methods explaining what they do.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D78614