[PATCH] D72885: [WIP] Add Query API for llvm.assume holding attributes

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added inline comments.


================
Comment at: llvm/include/llvm/Transforms/Utils/KnowledgeRetention.h:76
 
+struct SetPreserveAllScope {
+  bool PreviousValue;
----------------
Documentation missing.


================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:169
+  assert(Assume.getIntrinsicID() == Intrinsic::assume);
+  assert(isExistingAttribute(AttrName));
+  assert((ArgVal == nullptr ||
----------------
Commonly we have messages in an `assert`, also below.


================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:194
+  /// The elements in the operand bundle are sorted by Tag then the Name of the
+  /// value they are on then by increasing order of argument value.
+  if (AQR == AssumeQuery::Lowest)
----------------
I mentioned in D72475 now that the name should not be a part of the key as it can change.


================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:221
+    *ArgVal = cast<ConstantInt>((Assume.op_begin() + Lookup->Begin + 1)->get())
+                  ->getZExtValue();
+  return true;
----------------
The above seems complicated. Can you use helpers or comments to make it easier to read?


================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:239
+  ShouldPreserveAllAttributes.setValue(PreviousValue);
+}
----------------
Where is this used?


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

https://reviews.llvm.org/D72885