It still fails some tests and I'm afraid it changed something since the review was almost three years ago.
I don't have enough time to work on so @xbolva00, could you take over this PR if you have urgent use cases?

Rebased. I didn't realize this was not merged. Sorry everyone who waited for this.

We could always make it an argument for the pass, e.g., when you create it you select the precision. Or, a command line option to get higher/lower precision. I think by default it should track all features and we can have subsets defined in a command line option if necessary. So if a user wants only 10 of the 40 features they add a selector into a command line enum and use it to skip features.

Add setters/getters and rename the class name.

We could always make it an argument for the pass, e.g., when you create it you select the precision. Or, a command line option to get higher/lower precision. I think by default it should track all features and we can have subsets defined in a command line option if necessary. So if a user wants only 10 of the 40 features they add a selector into a command line enum and use it to skip features.

Creating an argument for the pass sounds good for me. IMHO, we don't need command option for the pass because I guess all use cases are from LLVM internal.

And if you wish to add other features not listed here, please let me know.

@jdoerfert @mtrofin
Currently, I created a new pass because I'm not sure additional costs to calculates new features are affordable at MLInliner.
I'd like to put new features on FunctionPropertiesInfo if computational costs don't matter.

Basically it looks good to me.
However, it's not obvious to me that (known) willreturn can derive mustprogress.
(It seems correct to me though)

Handle Global Value

Small fix

LGTM

Sorry, I quite forgot this patch...
@bollu Yes, that's fine. Thanks!

Great, LGTM :)

Why can't we take a union of assumed range?

In D77752#2215788, @mtrofin wrote:

In D77752#2215746, @uenoku wrote:

Hi, I want to use my TensorFlow model in LLVM with MLModelRunner, like ReleaseModeModelRunner.cpp but I have difficulty compiling the model, especially in the TensorFlow AOT complication of the model.
It would be really helpful if you point me out the code which defines the computation graph and the signature. Is such a script already landed or published somewhere?

We are about to publish the training algorithm, I'll update here when that's done - meanwhile, a possible way to unblock would be to inspect the expected inputs and outputs of the graph from the protobuf itself - it is in textual form - or using tensorboard, I believe it has a facility for this.

Note that the only interface between LLVM and the model is the input and output signatures, the internal structure of the model can differ.

Hi, I want to use my TensorFlow model in LLVM with MLModelRunner, like ReleaseModeModelRunner.cpp but I have difficulty compiling the model, especially in the TensorFlow AOT complication of the model.
It would be really helpful if you point me out the code which defines the computation graph and the signature. Is such a script already landed or published somewhere?

What happened to this patch?

In D83185#2134452, @kuter wrote:

In D83185#2134444, @uenoku wrote:

In D83185#2134395, @kuter wrote:

In D83185#2134275, @uenoku wrote:

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

Yes, I think replacing ZeroOrMore with OneOrMore is enoguh

I think cl::OneOrMore causes a error to be generated if the option is not specified.
That would make it mandatory to have a atleast one --attributor-seed-allow-list parameter for every invocation of
opt

https://llvm.org/docs/CommandLine.html

The cl::OneOrMore modifier indicates that the option must be specified at least one time.

In D83185#2134395, @kuter wrote:

In D83185#2134275, @uenoku wrote:

Either is fine but I think it is more natural to forbid an empty list.

Do you mean returning a error if a empty --attributor-seed-allow-list option is present ?
Currently the size of list is being used to tell if a list is present or not.
I think I can use getNumOccurrences() to replace this behaviour .

Either is fine but I think it is more natural to forbid an empty list.

MaxPotentialValues is defined multiple times if you put the definition to the header.
You have to make it an external variable or declare in AttributorAttributes.cpp

I think replacing all makes the code hard to understand (because "call base" is just a name of LLVM class)
Personally, I prefer maintaining "call site"

Looks good to me.

Looks reasonable to me.

LGTM

LGTM

This makes sense to me.

In D76674#1944353, @lebedev.ri wrote:

In D76674#1944250, @uenoku wrote:

Why can't we use ABI info without known access?
I mean, is it possible to use ABI info in initialize?

See my previous comment.
If we decide that pointer has ABI alignment from the get go,
then we will immediately update every underaligned load/store to no longer be underaligned,
which makes no sense. Unless i'm horribly mistaken, just because we said that %z is i32* or i512*,
it doesn't mean it is UB for it to be aligned to a single byte (& 0b1 == 1),
it is the access to such misaligned pointer that is UB.

Why can't we use ABI info without known access?
I mean, is it possible to use ABI info in initialize?

Good point. It is about:
Attribute::StructRet, Attribute::ByVal, Attribute::InAlloca, Attribute::InReg, Attribute::Returned, Attribute::SwiftSelf, Attribute::SwiftError, Attribute::Alignment

Seems reasonable but I'm not sure why we need to care about returned.

LGTM but it is not fixing the typo rather refactoring :)

LGTM

test?

I think Stefan is correct. AnalysisGetter provides such functionality.

void use(int *);
__attribute__((noinline)) static void f1(int * restrict p, int c1, int c2) {
  if (c1) {
    use(p);
  }
  if (!c2) {
    use(p);
  }
}
void f2(int * restrict p, int c) { f1(p, c, c); }

Could you add this example to test?
In this case, c1 and c2 are always same. Because of flow sensitivity, the first callsite is not reachable to the second callsite. So %p is noalias at both callsites. It can't be deduced with isPotentiallyReachable so please add FIXME tag.

Please upload full context of the diff (diff -U999999)

Thank you for working on this! Please add isKnownReachable change.

Thank you for working on this!!

LGTM

In D74691#1922032, @fhahn wrote:

In D74691#1921957, @uenoku wrote:

You don't have to care about the failure if the failure doesn't occur multiple times.
Please see https://lists.llvm.org/pipermail/llvm-dev/2019-November/136710.html

I think a better way to put it is that you don't have to care about the failure if you did not cause it (e.g. you might not get multiple emails if other bots were broken before)

You don't have to care about the failure if the failure doesn't occur multiple times.
Please see https://lists.llvm.org/pipermail/llvm-dev/2019-November/136710.html

LGTM

I think the title should be more descriptive, like "improve noalias preservation using reachability" or something.

LGTM

Please guide on why nonull.ll shows additional noalias propagation. I see that the caller doesn't have noalias attribute but it has derived it.

It was caused by a bug that the dependence track was missing for NoAliasAA so I have pushed the fix. (https://github.com/llvm/llvm-project/commit/d9bf79f4e9952acca1fa353e39bcee89cd69550f)

This patch traverses the same context multiple times so the cost might get huge.
I'll create something like a cache.

I have misunderstood how CaptureTracker works. AANoCapture already has this functionality :) So I'd close it.
But I have thought some of FIXME in the test seems wrong.

I have missed to check the dereferenceability of the pointer. I'll fix it later.
So now the tests are broken.

Please use code block.

I have not convinced myself that this covers the recursive case properly, maybe that was never the intend though. Can we have a test like this:

Fix comment and add test.

As I see the discussion in the forum, it seems best to create a basic block for each case. I'll implement so.

In D75433#1900576, @ftynse wrote:

We can impose a similar restriction on switch in the LLVM dialect and require whoever constructs the IR to deal with it. This may be suboptimal from the usability standpoint, but so is automagically inserting dummy blocks in the translation. We've had a similar discussion in the indirectbr patch, inconclusive as far as I can tell. Let's bring this to the discussion form.

Ok, that seems fine.

@rriddle @ftynse Thank you for the comments :)

In D75433#1900487, @ftynse wrote:

Thanks for the patch!

Currently, successors are not allowed to have block arguments because we can't directly translate block arguments into phi node.

Could you elaborate on this? We have logic that translates branch arguments into phi nodes as a post-processing step after all blocks, it should be sufficient to modify getPHISourceValue to handle switch in addition to branches.

Simplify implementation.

Could you make sure that you are using a new pass manager? (It means to use ./opt -passes=attributor .. but not ./opt -attributor ..)
If you are using an old pass manager, you might not get LoopInfo or other analysis.

Address comments

Rebased. I simply use AbstractState to accumulate the known information.