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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:18
+using namespace llvm;
+using namespace detail_assume_builder;
+
----------------
Tyker wrote:
> 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.*
Then move it into the cpp file and only declare functions that deal with its members in the header.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:24
+cl::opt<bool> GroupAssumeBuilderPrinter("assume-builder-printer-group",
+                                        cl::ReallyHidden, cl::init(false));
+
----------------
Tyker wrote:
> 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.
Oh, why do we need a printer if we could actually do the tranformation? We can use unit tests to test this.


================
Comment at: llvm/test/Transforms/Util/assume-builder.ll:55
+  ret void
+}
----------------
Tyker wrote:
> 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.
> 
Yes the function declaration attributes, e.g., nounwind. 

The use case I have in mind would be to create an `llvm.assume` if you are about to move/delete/modify the call.
Having information about the arguments helps then, having information about the function does not, except if we would have a scope in which we know the information is good. I mean, if you inline a call site with an attribute XYZ but these attributes are all "scoped". Knowing XYZ holds at a single location is not helpful, at least I can't think of many function attributes for which this would be helpful in a non-scoped case. 


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

https://reviews.llvm.org/D72475