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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/AssumeBuilder.h:24
+  return BuildAssumeFromInst(I, I->getModule());
+}
+
----------------
Mention that the instruction is not inserted into a basic block.


================
Comment at: llvm/include/llvm/Transforms/Utils/AssumeBuilder.h:31
+public:
+  explicit AssumeBuilderPass() {}
+
----------------
all but the run method don't seem to be needed (if this is a struct).


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:25
+
+  SmallSet<std::tuple<const char *, Value *, Value *>, 8> BundleSet;
+
----------------
I usually favor structs with named members over tuples. It makes especially the uses way more readable, e.g., it says `Elem.Name` not `Elem.get<0>()`.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:29
+
+  void AddAttribute(AssumeBuilderState &Builder, Attribute Attr, Value *WasOn) {
+    StringRef Name;
----------------
No need for `Builder`, this is a member. Same elsewhere.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:44
+    for (AttributeList AttrList :
+         {Call->getAttributes(), Call->getCalledFunction()->getAttributes()}) {
+      /// the start Index is at 1 because 0 is the return value.
----------------
getCalledFunction might be null.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:45
+         {Call->getAttributes(), Call->getCalledFunction()->getAttributes()}) {
+      /// the start Index is at 1 because 0 is the return value.
+      for (unsigned Idx = 1; Idx < AttrList.getNumAttrSets(); Idx++)
----------------
don't use these constants but `AttributeList::ReturnIndex`, `AttributeList::FunctionIndex`, `AttributeList::FirstArgIndex`.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:55
+  Instruction *Build() {
+    if (!BundleSet.empty()) {
+      Function *FnAssume = Intrinsic::getDeclaration(M, Intrinsic::assume);
----------------
Early exist are preferred: 
```
if (XXX.empty())
  return nullptr;
/// other code
```


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:59
+      SmallVector<OperandBundleDef, 8> OpBundle;
+      for (auto Elem : BundleSet) {
+        SmallVector<Value *, 2> Args;
----------------
I'd prefer the type instead of `auto`, which is simple if we make it a struct, but either way it should be `const XXXX &` whenever possible.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:77
+  }
+};
+
----------------
Formatting: The methods should start with a lower case letter.


Maybe add a TODO mentioning that we should look in the vicinity for other assumes we can merge the information with.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:85
+  return Builder.Build();
+}
+
----------------
I'd just get the module from the instruction. `I->getModule()`


================
Comment at: llvm/test/Transforms/Util/assume-builder.ll:55
+  ret void
+}
----------------
Tyker wrote:
> jdoerfert wrote:
> > 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. 
> i think most function attributes aren't useful. but some are for example cold if we hit a call to a function marked cold, this is useful.
> and we can't differentiate between useful and useless ones.
Sure, let's keep them for now.

---

We need a test in which a property holds for multiple arguments.


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

https://reviews.llvm.org/D72475