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

[Tyker via Phabricator via llvm-commits]

Tyker marked 5 inline comments as done.
Tyker added inline comments.


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


================
Comment at: llvm/lib/IR/Attributes.cpp:201
+}
+
 //===----------------------------------------------------------------------===//
----------------
jdoerfert wrote:
> This can probably go in the other patch, right?
getNameFromAttrKind is used in the to build the assume. but it could be moved in the previous patch.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:18
+using namespace llvm;
+using namespace detail_assume_builder;
+
----------------
jdoerfert wrote:
> Why do we need the `detail_assume_builder` namespace at all?
it can be probably be removed. but i consider builderState a "common" name and the builderState shouldn't be used outside of AssumeBuilder.*


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:24
+cl::opt<bool> GroupAssumeBuilderPrinter("assume-builder-printer-group",
+                                        cl::ReallyHidden, cl::init(false));
+
----------------
jdoerfert wrote:
> 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`.
it is just intended to be used for testing of the range version of the function.
the pass doesn't insert instruction it just output what it would look like if it did.

i made it look the same so i can use python scripts to update tests. but instruction are not inserted.


================
Comment at: llvm/test/Transforms/Util/assume-builder.ll:55
+  ret void
+}
----------------
jdoerfert wrote:
> 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.
> Why is the address space explicit here?

i think it is because the llvm.assume are not inserted but printed as if they where. so the printing option may be different.

> 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.

by `function (scope) attributes` do you mean attributes on the argument of the declaration of a function ?
if so i think they can give us information about the call site. but if we can assume they are always propagated to the callsite we can skip them.



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

https://reviews.llvm.org/D72475