[PATCH] D72475: [WIP] Build assume from call

[Johannes Doerfert via Phabricator via llvm-commits]

jdoerfert accepted this revision.
jdoerfert added a comment.
This revision is now accepted and ready to land.

In D72475#1817585 <https://reviews.llvm.org/D72475#1817585>, @Tyker wrote:

> In D72475#1817423 <https://reviews.llvm.org/D72475#1817423>, @jdoerfert wrote:
>
> >   `bool getAttributeFromAssume(Value &Base, CallInst &AssumeCI, Attribute::Kind Kind, int &Value)`
> >
> > and similar for string attributes. Alternatively we could return a map from `WhatOn` values to attributes.
>
>
> something like that seem fine.
>
> > I don't understand. What do you mean by "good names"? We have the operand bundle strings, right?
>
> I meant that in the operand bundle the first argument is the WasOn and the second is optionally the attributes value.
>  but users shouldn't have to use magic numbers to access those fields.


Oh, right, agreed!

>> Agreed that an API should hide the actual encoding, thus the above grouping is something we should be able to do easily.
> 
> i don't know if the grouping is desirable it makes arguments in the bundle more confusing when there is the attribute has an optional argument.
>  but i wanted to have the opportunity to tweak the representation later.
> 
> is there any comment left on this patch ?

I added some minor ones but I think this patch is otherwise fine. We should not land it right away though. If you write the API next, either of us can make use of it in the Attributor. Lastly we need to generate the calls in the first place and we can try this out on benchmarks.



================
Comment at: llvm/include/llvm/Transforms/Utils/AssumeBuilder.h:20
+/// Build an instruction to preserve informations that can be derived from
+/// this it. if no information was found, returns null.
+/// The returned instruction is not inserted anywhere.
----------------
Nit: Maybe say we return an assume call instead of just "instruction"? 

The sentence is broken I think, the end should be something like `derived from \p I.`


================
Comment at: llvm/include/llvm/Transforms/Utils/AssumeBuilder.h:25
+  return BuildAssumeFromInst(I, I->getModule());
+}
+
----------------
Arguably, you only need to expose one version, e.g., the single instruction one (with const). In the impl. we can query the module.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:7
+//
+//===----------------------------------------------------------------------===//
+
----------------
Here and in the header we should add a short file comment describing what functionality is in here.


================
Comment at: llvm/lib/Transforms/Utils/AssumeBuilder.cpp:17
+#include "llvm/Support/CommandLine.h"
+#include "llvm/Support/FormattedStream.h"
+
----------------
Nit: We might not need all of these anymore.


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

https://reviews.llvm.org/D72475