[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/lib/Transforms/Utils/KnowledgeRetention.cpp:251
+  return true;
+}
+
----------------
Tyker wrote:
> jdoerfert wrote:
> > Tyker wrote:
> > > jdoerfert wrote:
> > > > What is the use case for `IsOn = nullptr`? If there is none, we should make it a reference so it's obvious. 
> > > currently `IsOn = nullptr` is for attributes that where on the call of a function or the declaration of a function.
> > > 
> > > this case only occurs when ShouldPreserveAllAttributes is set.
> > I would have expected the IsOn to be the called value or respectively function then. Is there a reason for one or the other?
> i am staring to think the attribute on call and function is perhaps too small of a use case to be worth supporting. i haven't yet found any other use case than the cold attribute that said i haven't be looking for it. 
> 
> but the idea was that Users of that API wouldn't need to care or know what function was called here. but just that this function has X attribute. i think it loose its last bit of usefulness if users need to know ahead of time that there was a calls to function F before querying it.
I can see your argument. Maybe the "map API" will allow your use case even if we retain the functions that carried the attributes.
I am hesitant to throw away information here if there is another way. When we query *all* information the assume stores in a map the keys could either be attributes or values (probably both make sense). In the keys = attributes version the user can look for "cold" and act on it. Arguably, we don't even need the map API for this. We can have a define that if `IsOn` is `null` any occurrence of the attribute is returned even though we store the original "IsOn" value internally.


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

https://reviews.llvm.org/D72885