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.

Address comments

And I have a question.

define i32 @f1(i32* %p, i32* %q){
entry:
  %0 = load i32, i32* %q
  %1 = load i32, i32* %p
  %add = add nsw i32 %1, %0
  ret i32 %add
}

In D74691#1878429, @jdoerfert wrote:

In D74691#1878377, @uenoku wrote:

In D74691#1878376, @jdoerfert wrote:

In D74691#1878373, @uenoku wrote:

I'm not sure whether we can say if all the loops given by LoopInfo have a max trip count and all the callees are willreturn, then we can guarantee the termination of that function.

We can not. But if all cycles in the CFG are loops recognized by loop info and they have a max trip count we are good (in this part), I think.

How can we confirm all cycles in the CFG are recognized actually by loop info? Is there a simple way?

What @omarahmed does here should work (with the fixes I mentioned). As before, we identify all cycles via the depth-first search and the visited set. If a cycles is found we did bail before but now we ask LI & SE instead. If they say we found a proper loop header of a loop with a bounded trip count we can ignore that cycles and continue exploring. Do you think there is a problem with that logic?

I might misunderstand the logic. This seems fine. I also think @baziotis's idea works well.

In D74691#1878376, @jdoerfert wrote:

In D74691#1878373, @uenoku wrote:

I'm not sure whether we can say if all the loops given by LoopInfo have a max trip count and all the callees are willreturn, then we can guarantee the termination of that function.

We can not. But if all cycles in the CFG are loops recognized by loop info and they have a max trip count we are good (in this part), I think.

How can we confirm all cycles in the CFG are recognized actually by loop info? Is there a simple way?

I'm not sure whether we can say if all the loops given by LoopInfo have a max trip count and all the callees are willreturn, then we can guarantee the termination of that function.
Aren't there other factors causing non-terminating execution?