[PATCH] D72475: [WIP] Build assume from call

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Some high-level comments, haven't gone through it in detail



================
Comment at: llvm/include/llvm/Transforms/Utils/AssumeBuilder.h:30
+  void AddInstruction(const Instruction *I);
+};
+
----------------
We need to add documentation and probably change the variable/function names to match the convention.


================
Comment at: llvm/lib/IR/Attributes.cpp:201
+}
+
 //===----------------------------------------------------------------------===//
----------------
This can probably go in the other patch, right?


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:18
+using namespace llvm;
+using namespace detail_assume_builder;
+
----------------
Why do we need the `detail_assume_builder` namespace at all?


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:24
+cl::opt<bool> GroupAssumeBuilderPrinter("assume-builder-printer-group",
+                                        cl::ReallyHidden, cl::init(false));
+
----------------
Since this is not always sound I am unsure what it would be used for.
Instead, you can look for assumes in the vicinity, probably always prior to the current position, but with consideration of `willreturn` and `nounwind`.


================
Comment at: llvm/test/Transforms/Util/assume-builder.ll:55
+  ret void
+}
----------------
Why is the address space explicit here?

---

I'm unsure if we need to preserve function (scope) attributes, at least until we get scoped assumes that actually scope the information, e.g. after inlining.


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

https://reviews.llvm.org/D72475