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

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert added a comment.

In D72885#1832612 <https://reviews.llvm.org/D72885#1832612>, @Tyker wrote:

> 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.


Great! Can you share it on phab so I can reference it in my email (see below).

> 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.

Correct. I'll send another email today, describing our progress and hoping ppl agree this is a good solution :)



================
Comment at: llvm/lib/Transforms/Utils/KnowledgeRetention.cpp:239
+  ShouldPreserveAllAttributes.setValue(PreviousValue);
+}
----------------
Tyker wrote:
> 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.
Now I understand but I still think this is problematic.

What if we add a second call into KnowledgeRetention.h where the value of  `ShouldPreserveAllAttributes` is passed in directly. The existing interface will call that internally and use the command line flag value. Instead of the command line flag value, existing uses will then use the new argument. That way we can determine the value from the call site or via the command line.

What do you think?


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


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

https://reviews.llvm.org/D72885