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

[Tyker via Phabricator via llvm-commits]

Tyker marked 2 inline comments as done.
Tyker added a comment.

In D72885#1827778 <https://reviews.llvm.org/D72885#1827778>, @jdoerfert wrote:

> I won't review this properly till next week. Generally looks good so you can work on the next patch if you want to.


i started working on ignoring or dropping uses in llvm.assume when a transformation is use sensitive.

In D72475#1821013 <https://reviews.llvm.org/D72475#1821013>, @jdoerfert wrote:

> Also because of that, and we do not have settled on this scheme in the RFC thread.


i would like to have confirmation that the operand bundles representation is what we are going for ?
i don't recall it being a point of contention but it wasn't explicitly approved. and you mentioned in that the issue as not settled.



================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:239
+  ShouldPreserveAllAttributes.setValue(PreviousValue);
+}
----------------
jdoerfert wrote:
> Tyker wrote:
> > jdoerfert wrote:
> > > Where is this used?
> > this is used for testing ShouldPreserveAllAttributes option from unittests.
> If it's only used in the unit test can we move it there?
it needs to have access to the ShouldPreserveAllAttributes flag. which we probably shouldn't make global.


================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:251
+  return true;
+}
+
----------------
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.


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

https://reviews.llvm.org/D72885