Thank you.

In D84980#2185839, @jdoerfert wrote:

Really cool :) LGTM

In D84722#2181037, @jdoerfert wrote:

Can you, maybe as a separate patch, run the update script with the call site deduction enabled *before* this patch and *after* to get the difference on all tests we have?

add auto context injection.

Some Attributes that have CBContext doesn't seem to be querying the CB specific attributes.
I will update this once I have a solution.

remove unused header.

add unit test.
fix bug (😄)
fix typo.

Manually requesting review, since the Phabricator wasn't able to built this. From what I can see it was missing the parent revision with the last build.
Pre-merge checks should be ok now.

bug fix.
add range contents as test.

fix tests.

add missing code.
remove unneeded definies.
fix bug.

In D81929#2172319, @jdoerfert wrote:

LGTM with nits.

fix styling.

Add unittest.

Memory usage for this patch:
comand: $LLVM_BIN/opt -S -attributor-cgscc test-suit/CTMark/SPASS/clause.ll
before:

calls to allocation functions: 78794 (82506/s)
temporary memory allocations: 3942 (4127/s)
peak heap memory consumption: 2.58MB
peak RSS (including heaptrack overhead): 355.73MB
total memory leaked: 140.33KB

after:

calls to allocation functions: 78802 (86977/s)
temporary memory allocations: 3942 (4350/s)
peak heap memory consumption: 2.59MB
peak RSS (including heaptrack overhead): 356.60MB
total memory leaked: 140.33KB

difference:

calls to allocation functions: 8 (-163/s)
temporary memory allocations: 0 (0/s)
peak heap memory consumption: 8.26KB
peak RSS (including heaptrack overhead): 0B
total memory leaked: 0B

Interesting, there doesn't seem to be much of a difference.

In D81929#2160764, @jdoerfert wrote:

All this is missing now is a test :). We can just check for the debug output.

fix wording. use pointer_iterator instead of for loop.
fix naming.

small improvements.

add missing documentation.
improve formating and readability.
fix typos.
rename command line option

fix run lines (there was a mistake in the earlier revision).

Add test combinations.

In D83744#2158768, @jdoerfert wrote:

Some minor comments below.

Upload diffs with full context please.

fix typo.
improve code readability.

add attributor-enable-call-site-specific=false as a test.
fix the run lines.

In D83744#2150057, @jdoerfert wrote:

Cool! As discussed, merge D83299 and add new call sites for test1.

merge with D83299
add positive and negative tests.

merged with D83744

In D83299#2140135, @jdoerfert wrote:

Can we include some code that triggers this so we can tests it?

D83744 uses this and has tests.

Fix typo

Hi @bbn when will commit this ?

In D82861#2146253, @okura wrote:

In D82861#2146246, @sstefan1 wrote:

In D82861#2146242, @okura wrote:

In D82861#2137685, @jdoerfert wrote:

Can you merge this?

Do I have a right to merge this by myself? I did arc patch and tried to git push https://github.com/llvm/llvm-project.git HEAD:master according to the document, but I failed to do that.

Did you get the commit access? If so, what problems did you have with git push?

I got the following message with git push

remote: Permission to llvm/llvm-project.git denied to okuraofvegetable.
fatal: unable to access 'https://github.com/llvm/llvm-project.git/': The requested URL returned error: 403

From this message, it seems to me that I don't have commit access.

1. Better comments.
2. Remove unneeded lines from the test.

In D83297#2138744, @bbn wrote:

Can we just merge the dependency graph and the "SchedulingGraph" because I think they are quite similar.

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

In D83185#2134275, @uenoku wrote:

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

In D78861#2133485, @bbn wrote:

In D78861#2131573, @jdoerfert wrote:

This looks pretty good :). Nice active review :)

I have some minor comments below. We also should add a test for the print and dot output.

I need some help here:
Is there a way to test the dot output? I checked the .dot file and found it hard to write CHECK lines (see below) because we are interested in the link between different graph nodes (line 3 and line 4)

	Node0x55be15e4f7d0 [shape=record,label="{[AAValueSimplify] for CtxI '  %2 = load i32, i32* %0, align 4' at position \{arg: [@0]\} with state simplified\n}"];
	Node0x55be15e4f810 [shape=record,label="{[AANoUnwind] for CtxI '  %2 = load i32, i32* %0, align 4' at position \{fn:checkAndAdvance [checkAndAdvance@-1]\} with state nounwind\n}"];
	Node0x55be15e4f810 -> Node0x55be15e500b0;
	Node0x55be15e4f810 -> Node0x55be15e500b0;

I have referred to some other similar tests like the *cfg_deopt_unreach.ll*, but none of theme shows how to write check lines for such testcases.

Introduce test for the command line option

Replace ternary expression with early return

In D83172#2131783, @sstefan1 wrote:

In D83172#2131779, @bbn wrote:

Attributes are going to be decleared outside of the Attributor soon.

Do you mean that there might be other abstract attributes defined outside the AttributorAttributes.cpp ?

As of yesterday, there is an AA outside of AttributorAttributes :). You should rebase.

so I think we can have a macro like this:

#define ATTRIBUTE_BOILER(CLASS)                    \
    const char *getIdAddr() { return &ID}                 \
    const std::string getName() { return #CLASS;}   \
    static const char ID;

Yes this could work, but I am still unsure about it..... I'd also like to hear what others think about this.

I mean it is not that much code. Maybe I'm missing something, but why not just use ID directly?

In D83172#2131751, @bbn wrote:

@kuter Do you mean we can use macro inside the header file? for example, we create such macro and use it inside the declaration of the class, but I still think it is not clean.

This seems like too much code duplication.
I think we can have a macro to declare getIdAddr and the getName function.
I am curious what other people think about this.

Btw this patch introduces 8 bytes per attribute because of the CBContext.
I think we would ideally move it to a map inside the attributor.

general bug fix.

In D81929#2129517, @jdoerfert wrote:

In D81929#2129334, @kuter wrote:

In D81929#2128955, @jdoerfert wrote:

In D81929#2096257, @jdoerfert wrote:

We need some way of testing this, so tests that enable this and some way of checking the output. We could look for the debug output or add some other way of checking

Reverse ping. Unit tests might also be an option.

Wouldn't just testing the uses of this path enough.
For example if we have are getting a correct call site constant range would that be enough ?

Is the CBContext already used? I might have to take another look at this, sorry.

In D81929#2128955, @jdoerfert wrote:

In D81929#2096257, @jdoerfert wrote:

We need some way of testing this, so tests that enable this and some way of checking the output. We could look for the debug output or add some other way of checking

Reverse ping. Unit tests might also be an option.

Hi can you rebase this.

In D82625#2116406, @uenoku wrote:

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"

TODO: comments

@bbn Soo, I just realized that this way of implementing GraphTraits might be problematic with
graph iterators like (scc_iterator, df_ iterator) since they require a single entry point to correctly handle
disconnected graphs and the Attributor dependencies are disconnected. Most disconnected graphs use a "synthetic node"
because of this.

What do you mean by "put the GraphTraits in llvm namespace"?

In D78861#2081311, @kuter wrote:

This does not compile for me. The compiler error that I get is about creating a GraphTraits specialization outside of the llvm namespace.
When i put the GraphTraits specializations in llvm namespace it does compile.

But when I run it I get a segfault.

This does not compile for me. The compiler error that I get is about creating a GraphTraits specialization outside of the llvm namespace.
When i put the GraphTraits specializations in llvm namespace it does compile.

add newline after function declaration

1. rename ::runScheduler to ::runTillFixpoint
2. improve comments for ::runTillFixpoint

Hi, what is the state of this ?
As of D78729 a AbstractAttribute keeps track of its own dependencies.

In D76208#1956335, @jdoerfert wrote:

Apologies for the delay, can you rebase this and provide me with "Firstname Lastname <email>" from you so I can attribute it to you?

Rebased.
Small logic change in GEPOperator::accumulateConstantOffset to bailout on scalable vector types
except for when the offset is zero.
Not allowing zero breaks @test_accumulate_constant_offset_vscale_zero

Simplfy accumulateConstantOffset

Eliminated redundant debug messages, Style fixes, Added negative test case,
Don't use external analysis if the get operand is a struct type.