minor change

address comment

add test

delete unnecessary brackets

• fix dependence
• fix formatting

fix OpenMP/parallel_deletion.ll

Rebase

Reflect change in D86835

Delete AAIsDead query before manifestation because other patches (D86635, D86734) make it possible to query new AAs in manifest.

This is accepted, I'll go with this.

Another way to achieve the same goal is to remove the assertion in manifestAttributes.
I'm not sure which is better.

• delete debug print
• add a comment

• make use of the phase flag

• use scoped enum
• move status change into Attributor::run

In D86678#2241129, @kuter wrote:

Thank you for creating this. I was thinking of something like this rather than having multiple flags.
In D86587 I iterate on functions multiple times and I delete AAs early after manifesting them.
(Manifest happens multiple times).
I think most of the state change should happen in the ::run function.

In D86678#2241149, @lebedev.ri wrote:

Patch is missing description.

• fix initialization at callsite argument position (value-simplify.ll is affected)
• add tests

In D85592#2236740, @jdoerfert wrote:

What happens if you have something like

int foo(int c0, int c1, int c2, int c3) {
   int undef;
   int x0 = c0 ? 0 : 1;
   int x1 = c1 ? x0 : undef;
   int y2 = c2 ? 0 : 7;
   int z3 = c3 ? x1 : y2;
   return z3 < 7;
}

int bar(int c0, int c1 ) {
   int undef;
   int x0 = c0 ? 0 : 1;
   int x1 = c1 ? x0 : undef;
   return x1 == 7;
}
int bar(int c0, int c1 ) {
   int undef;
   int x0 = c0 ? 0 : undef;
   int x1 = c1 ? x0 : 1;
   return x1 == 7;
}

In D86635#2239738, @kuter wrote:

I think rather than creating a new api function, it would be more robust if we had a flag as member of the attributor.
It would be similar to SeedingPeriod flag that I have added.

Simply we would make it true in the Attributor::manifestAttributes function and then we would have a assertion in Attributor::updateAA

Thank you for the suggestion. I'm thinking about the same approach. I totally agree with @kuter. It is slightly hard for us to guarantee Attributor not to update AAs during manifestation by adding a new API.
Adding a flag that indicates which stage of the process (e.g. the update stage or the manifestation stage) we are in is better, I think.
@jdoerfert How do you think about this?

Some AAs use checkForAll... in manifest, and getAAFor is used in the function.
I think we need to add a new parameter like bool DuringManifest to them in a follow-up.

• rename getter getAADuringManifestFor
• replace some existing getAAFor in manifest with this

In D86565#2238954, @okura wrote:

In D86565#2238813, @sstefan1 wrote:

Are you going to move the AAIsDead query to the update?

No, I'm going to allow to query (initialize) new AA in the manifestation stage. That will save us from querying AAIsDead before the manifestation stage.

In D86565#2238813, @sstefan1 wrote:

Are you going to move the AAIsDead query to the update?

In D86396#2236684, @jdoerfert wrote:

In D86396#2236597, @okura wrote:

When I fix the identification of AANoUndef in identifyDefaultAbstractAttributes, I noticed that noundef should not be deduced for dead argument and dead call site argument position too. (I found wrong transformation in e.g. callbacks.ll)
Not to deduce for dead positions, we should not indicate optimistic fixpoint in initialize because once fixpoint is indicated we cannot change the state. However, it weakens the deduction significantly.
So I want to suggest that we check not only noundef but also liveness in AAUndefinedBehavior.

I don't understand the comment.
Should we check in the manifest of AANoUndef if the position is dead and not manifest it?

I investigated the problem seen in the module slice patch again. And I realize now that checking liveness in manifestation is sufficient to settle the problem.
I misunderstood we need more things to do so. I was overthinking, sorry.

This was split into some patches.

address comments

• fix nit

In D86565#2237393, @jdoerfert wrote:

In D86565#2237391, @okura wrote:

In D86565#2237383, @jdoerfert wrote:

In D86565#2237339, @okura wrote:

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

Sorry, I was wrong. CheckBBLivenessOnly should be false because if it is true we cannot catch the assumed liveness of e.g. returned position.
When I changed it to true, a new AAIsDead was registered in manifestation, and segfault happened. So I register them in initialize.

Don't do that but query them in the update instead.

I have difficulty in doing that because segfault happens when a fixpoint is indicated in initialize and updateImpl is not called even once.

TBH, I think we need to remove the assertion that you see (it should not be a segfault). And replace it with code that causes a pessimistic fixpoint for all attributes created after we are done with the update stage. They can initialize but that is it.

In D86565#2237383, @jdoerfert wrote:

In D86565#2237339, @okura wrote:

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

Sorry, I was wrong. CheckBBLivenessOnly should be false because if it is true we cannot catch the assumed liveness of e.g. returned position.
When I changed it to true, a new AAIsDead was registered in manifestation, and segfault happened. So I register them in initialize.

Don't do that but query them in the update instead.

I have difficulty in doing that because segfault happens when a fixpoint is indicated in initialize and updateImpl is not called even once.

• change CheckBBLivenessOnly to true
• add affected tests
• add new tests

In D86565#2237287, @jdoerfert wrote:

LGTM, one minor note. The test will be part of the module slice patch, I'm fairly certain this cannot be tested right now but it makes sense.

When I fix the identification of AANoUndef in identifyDefaultAbstractAttributes, I noticed that noundef should not be deduced for dead argument and dead call site argument position too. (I found wrong transformation in e.g. callbacks.ll)
Not to deduce for dead positions, we should not indicate optimistic fixpoint in initialize because once fixpoint is indicated we cannot change the state. However, it weakens the deduction significantly.
So I want to suggest that we check not only noundef but also liveness in AAUndefinedBehavior.

In D85592#2221775, @jdoerfert wrote:

In D85592#2221557, @fhahn wrote:

I am not sure if the following scenario is possible, but if it is it might be problematic when undef is unified with multiple values. E.g. we found that we have the following potential values 0, 1, undef. If we now assume this is 0, 1, we might simplify and %x, 3 to %x. But that is wrong, if %x is undef. Note that simplifications that result in a constant should be fine either way. As I said, not sure if the potential values could be used like that (possibly in the future).

It doesn't seem to be a problem now. Two results from the IRC discussion:

1. We need a test for the above situation, to make sure we don't regress this.
2. We should make sure when we merge the possible values with undef properly, e.g., if you intersect two sets of possible values {0,1,undef} and {0, 7} then we should get {0, 7}.

I'm not sure we have to take care of intersection. Currently, there is PotentialValueState::intersectWith but is not used anywhere. I am not able to come up with a case we need it, and I think it should be deleted.

• fix according to interface change

• change whole interface

• delete unnecessary format change

• change to check IR position

I think the logic is correct too and we need comments for this.
IMHO, putting the conditional into helper function would improve readability.