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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

Only minor points are left. including:
 Can we make string attributes and function scope attributes optional (under a command line flag)? Especially the uses created for the latter could easily confuse the call graph and similar passes. That said, we need to teach them to ignore llvm.assume uses (see below).

Next steps (in no particular order):

- Teach passes sensitive to uses, especially instcombine, to ignore uses in an llvm.assume.
- Teach passes, e.g., the Attributor, to use the information provided this way.
- Emit llvm.assume when we move/delete call instructions.
- Perform some experiments.
- Revisit the list to get agreement.



================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:77
+  }
+};
+
----------------
Tyker wrote:
> jdoerfert wrote:
> > 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.
> > Maybe add a TODO mentioning that we should look in the vicinity for other assumes we can merge the information with.
> to do this the BuildAssumeFromInst will have to change and insert the assume. i don't know if it is preferable.
> to do this the BuildAssumeFromInst will have to change and insert the assume. i don't know if it is preferable.

Fair point. We can investigate that in the future. Maybe a method to locate an existing assume, one to combine the information, one to create the new one and replace the old. Or a dedicated pass that moves them around and combines them. I think less assumes is "generally speaking" better but we don't need this now.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:42
+
+  SmallSet<Knowledge, 8> BundleSet;
+
----------------
Documentation for the class and members is missing. 

Nit: Maybe `KnowledgeSet` or sth. similar is a better name, `BundlSet` implies these are "already" operand bundles.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:70
+    if (Call->getCalledFunction())
+      addAttrList(Call->getCalledFunction()->getAttributes());
+  }
----------------
You can directly capture the result:
`if (Function *Fn = Call->getCalledFunction())`



================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:83
+        Args.push_back(Elem.Argument);
+      Args.push_back(Elem.WasOn);
+      OpBundle.push_back(OperandBundleDefT<Value *>(Elem.Name, Args));
----------------
Should we reverse the order here? That way the first argument is always the "WasOn" one.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:113
+  return PreservedAnalyses::all();
+}
----------------
Nit: one can use `for (Instruction &I : instructions(F))` to save one loop level here


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

https://reviews.llvm.org/D72475