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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

This looks for with minor comments and one remaining question: Do we use the value name in the sorting? If so, are we sure arbitrary renaming of values will not be a problem? If so, does it help to use the value name in the key?



================
Comment at: llvm/include/llvm/Transforms/Utils/KnowledgeRetention.h:60
+      AssumeCI, IsOn, Attribute::getNameFromAttrKind(Kind), ArgVal, AQR);
+}
+
----------------
Nit: Can you move the enum class (with a short doc) above the documentation of the function. It is a bit confusing this way and I'm not sure Doxygen will pick up on it they way we want it to.


================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:88
+            ? StringRef("")
+            : (*(Op.input_begin() + BOIE_WasOn))->getName());
   };
----------------
This still uses the value name in the key, right? Does this work if the values are renamed arbitrarily?


================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:194
+  assert((ArgVal == nullptr ||
+          Attribute::getAttrKindFromName(AttrName) == Attribute::None ||
+          Attribute::doesAttrKindHaveArgument(
----------------
Nit: Why do we allow `None` here?


================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:205
+    return (Assume.op_begin() + BOI.Begin + Idx)->get();
+  };
+
----------------
Nit: Move this down closer to the use sites. Maybe also replace `Elem` with `Value` to make it clear it returns an llvm value.


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

https://reviews.llvm.org/D72885