Page MenuHomePhabricator

[Attributor][IPConstantProp] Run the Attributor on IPConstantProp tests
ClosedPublic

Authored by jdoerfert on Nov 1 2019, 11:37 PM.

Details

Summary

The Attributor can, to some degree, do what IPConstantProp does. We can
consequently use the corner cases already collected and tested for in
the IPConstantProp tests to improve Attributor test coverage.

This exposed various bugs fixed in previous Attributor patches.

Not all functionality of IPConstantProp is available in AAValueSimplify
and AAIsDead so some tests show that we cannot perform the expected
constant propagation.

Diff Detail

Event Timeline

jdoerfert created this revision.Nov 1 2019, 11:37 PM
Herald added a project: Restricted Project. · View Herald TranscriptNov 1 2019, 11:37 PM
Herald added a subscriber: bollu. · View Herald Transcript
fhahn added a comment.Nov 2 2019, 2:03 PM

As mentioned in D69747 already, I am not too happy with the tests here to mix testing IPSCCP and IPConstantPropagation; I think it's valuable to just have a single target to run the relevant tests. Would it make sense to group the attributor tests together in a directory?

Also, unfortunately I did not have time to followd the attributor development closely lately, I am just curious: is there a benefit of doing some value simplifications in attributor directly?

As mentioned in D69747 already, I am not too happy with the tests here to mix testing IPSCCP and IPConstantPropagation; I think it's valuable to just have a single target to run the relevant tests. Would it make sense to group the attributor tests together in a directory?

I do not disagree but it is not as simple. I want to reuse existing test without copying them while the Attributor is still under development and needs to coexists with what we have. (It probably will stay that way for some passes we want to "share tests with", see below.) If we could use softlinks that would be great, otherwise I will probably ignore this minor issue and continue to run: llvm/test/Transforms/{IPConstantProp,FunctionAttrs,InferFunctionAttrs,ArgumentPromotion}

Also, unfortunately I did not have time to followd the attributor development closely lately, I am just curious: is there a benefit of doing some value simplifications in attributor directly?

TL;DR, I think so yes, at least in the short term. How we want to go forward once the Attributor matured fully is an open question.

There are reasons, but, as always, it is not a clear win. So perfect world, we have a single place with "IP-SCCP-like" logic that can make use of all "optimistic in-flight information" during a fixpoint iteration.
For now, we will have some IP-SCCP like logic rewritten in the Attributor framework, see for example D69605 for liveness improvements. The conditional folding logic needs cleanup before I put them here but that should happen in the coming days as well. Now we want this because we want/need liveness information during the deduction and we can use other information that is deduced to improve the value simplification part (think constant propagation) and thereby liveness. One example would be capture information and memory access information based on liveness information which feeds back into the rest.

As an actually unrelated example, this code (https://godbolt.org/z/vP-vvM) works with the Attributor while IP-SCCP for some reason dislikes the dead unknown(&X) call. In the Attributor framework, it doesn't matter because the use of X is simply never shown to the relevant AAValueSimplify class as long as we assume it is dead. (All Attributor::checkForAllXXXX(Predicate, ...) methods simply "hide" dead code. XXXX can be CallSites, ReturnValues, Uses,....)

static int X = 0;
void unknown(int *);

int foo(void) {
  if (X == 1)
    X = 2;  
  if (X == 2)
    unknown(&X);
  return X;
}
uenoku added a subscriber: uenoku.Nov 3 2019, 1:00 AM

What did we decide here? Can this go forward and we move tests after?

I don't think would be a big deal to just copy the tests: if you're using the test to check the behavior of a different pass, it isn't really the same test, even if the IR is identical.

I don't think would be a big deal to just copy the tests: if you're using the test to check the behavior of a different pass, it isn't really the same test, even if the IR is identical.

I'm fine with that but I'd prefer to invest the time only if I have confidence I get it in then.
@fhahn, @davide Is this OK with you or would you object?

fhahn added a comment.Nov 8 2019, 12:35 PM

Also, unfortunately I did not have time to followd the attributor development closely lately, I am just curious: is there a benefit of doing some value simplifications in attributor directly?

TL;DR, I think so yes, at least in the short term. How we want to go forward once the Attributor matured fully is an open question.

There are reasons, but, as always, it is not a clear win. So perfect world, we have a single place with "IP-SCCP-like" logic that can make use of all "optimistic in-flight information" during a fixpoint iteration.
For now, we will have some IP-SCCP like logic rewritten in the Attributor framework, see for example D69605 for liveness improvements. The conditional folding logic needs cleanup before I put them here but that should happen in the coming days as well. Now we want this because we want/need liveness information during the deduction and we can use other information that is deduced to improve the value simplification part (think constant propagation) and thereby liveness. One example would be capture information and memory access information based on liveness information which feeds back into the rest.

Would it be sufficient to have IPSCCP simplify the module before running the Attributor? Ideally it should apply the simplifications based on liveness (module cases it misses) and the attributor would not have to worry about them. I think it would be good try to avoid splitting development effort and keeping Attributor focused should simplify the way towards enabling it.

As an actually unrelated example, this code (https://godbolt.org/z/vP-vvM) works with the Attributor while IP-SCCP for some reason dislikes the dead unknown(&X) call. In the Attributor framework, it doesn't matter because the use of X is simply never shown to the relevant AAValueSimplify class as long as we assume it is dead. (All Attributor::checkForAllXXXX(Predicate, ...) methods simply "hide" dead code. XXXX can be CallSites, ReturnValues, Uses,....)

static int X = 0;
void unknown(int *);

int foo(void) {
  if (X == 1)
    X = 2;  
  if (X == 2)
    unknown(&X);
  return X;
}

This is interesting. I'll check what's going on with IPSCCP here. It might miss some global initializer facts.

With respect to the tests in this patch, I'd slightly prefer copying them to an attributor directory (this might avoid confusion on the future and makes it easier to ensure new tests added to the directory check the right thing), but it's also fine as is.

Also, unfortunately I did not have time to followd the attributor development closely lately, I am just curious: is there a benefit of doing some value simplifications in attributor directly?

TL;DR, I think so yes, at least in the short term. How we want to go forward once the Attributor matured fully is an open question.

There are reasons, but, as always, it is not a clear win. So perfect world, we have a single place with "IP-SCCP-like" logic that can make use of all "optimistic in-flight information" during a fixpoint iteration.
For now, we will have some IP-SCCP like logic rewritten in the Attributor framework, see for example D69605 for liveness improvements. The conditional folding logic needs cleanup before I put them here but that should happen in the coming days as well. Now we want this because we want/need liveness information during the deduction and we can use other information that is deduced to improve the value simplification part (think constant propagation) and thereby liveness. One example would be capture information and memory access information based on liveness information which feeds back into the rest.

Would it be sufficient to have IPSCCP simplify the module before running the Attributor? Ideally it should apply the simplifications based on liveness (module cases it misses) and the attributor would not have to worry about them. I think it would be good try to avoid splitting development effort and keeping Attributor focused should simplify the way towards enabling it.

My point is that liveness and other attributes are connected in known (and yet unknown) ways.

As an actually unrelated example, this code (https://godbolt.org/z/vP-vvM) works with the Attributor while IP-SCCP for some reason dislikes the dead unknown(&X) call. In the Attributor framework, it doesn't matter because the use of X is simply never shown to the relevant AAValueSimplify class as long as we assume it is dead. (All Attributor::checkForAllXXXX(Predicate, ...) methods simply "hide" dead code. XXXX can be CallSites, ReturnValues, Uses,....)

static int X = 0;
void unknown(int *);

int foo(void) {
  if (X == 1)
    X = 2;  
  if (X == 2)
    unknown(&X);
  return X;
}

This is interesting. I'll check what's going on with IPSCCP here. It might miss some global initializer facts.

With respect to the tests in this patch, I'd slightly prefer copying them to an attributor directory (this might avoid confusion on the future and makes it easier to ensure new tests added to the directory check the right thing), but it's also fine as is.

It seems copying is the favorite solution.
I'll create a new Attributor test directory and copy/move all relevant tests in there.
I will, if I do not hear complaints, do this without waiting for reviews as it will not impact anyone else.
Please let me know if you wish to have a pre-commit review for the test directory.

jdoerfert updated this revision to Diff 232760.Sun, Dec 8, 10:23 PM

Copy the tests into the Attributor test directory

jdoerfert updated this revision to Diff 232761.Sun, Dec 8, 10:28 PM

Fix the run and check lines

Build result: fail - 60619 tests passed, 9 failed and 726 were skipped.

failed: LLVM.Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/PR26044.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/arg-count-mismatch.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/musttail-call.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/openmp_parallel_for.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/remove-call-inst.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/return-argument.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/solve-after-each-resolving-undefs-for-function.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/user-with-multiple-uses.ll

Log files: console-log.txt, CMakeCache.txt

Build result: fail - 60619 tests passed, 9 failed and 726 were skipped.

failed: LLVM.Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/PR26044.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/arg-count-mismatch.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/musttail-call.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/openmp_parallel_for.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/remove-call-inst.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/return-argument.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/solve-after-each-resolving-undefs-for-function.ll
failed: LLVM.Transforms/Attributor/IPConstantProp/user-with-multiple-uses.ll

Log files: console-log.txt, CMakeCache.txt

jdoerfert updated this revision to Diff 232762.Sun, Dec 8, 11:07 PM

Properly rebase the tests

Build result: pass - 60628 tests passed, 0 failed and 726 were skipped.

Log files: console-log.txt, CMakeCache.txt

@fhahn @efriedma I copied the tests as discussed, can one of you accept?

Looks generally fine.

llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
25

This is technically a legal optimization given the test as written, but I don't think you're proving it correctly. readonly on a function does not imply readonly on its byval arguments. Try the following variation:

define internal i32 @vfu2(%struct.MYstr* byval align 4 %u) nounwind readonly {
entry:
  %z = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 1
  store i32 99, i32* %z, align 4
  %0 = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 1 ; <i32*> [#uses=1]
  %1 = load i32, i32* %0
  %2 = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 0 ; <i8*> [#uses=1]
  %3 = load i8, i8* %2
  %4 = zext i8 %3 to i32
  %5 = add i32 %4, %1
  ret i32 %5
}
llvm/test/Transforms/Attributor/IPConstantProp/musttail-call.ll
49

Comment isn't consistent with CHECK?

llvm/test/Transforms/Attributor/IPConstantProp/user-with-multiple-uses.ll
5 ↗(On Diff #232762)

This is the same test as remove-call-inst.ll?

jdoerfert marked 3 inline comments as done.Wed, Dec 11, 7:58 PM

Some context:
I did not write these tests but copied them and changed the run line. Given that was not the plan initially when we started the review I didn't want to just go ahead.
The plan now is to copy them, this patch, then run update_check on them, then use them to improve coverage once IPCP features trickle into the Attributor.

llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
25

The Attributor doesn't really do anything on this test yet. Only after pointer privatization goes in. I'll add your test case though.

llvm/test/Transforms/Attributor/IPConstantProp/musttail-call.ll
49

I'll get rid of the comment.

llvm/test/Transforms/Attributor/IPConstantProp/user-with-multiple-uses.ll
5 ↗(On Diff #232762)

It seem that way, I'll remove it.

efriedma added inline comments.Thu, Dec 12, 9:54 AM
llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
25

I probably should have posted a complete testcase. I meant taking this testcase, and replacing the vfu2 definition with my function. It looks like attributor transforms it. (Unless I messed up somehow.)

jdoerfert marked an inline comment as done.Thu, Dec 12, 11:06 AM
jdoerfert added inline comments.
llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
25

Here is the test, original and your version (_2), https://godbolt.org/z/A6L--T

What transformation do you mean and do you think it is wrong?

efriedma added inline comments.Thu, Dec 12, 12:11 PM
llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
25

The transform of the GEP from %z = getelementptr %struct.MYstr, %struct.MYstr* %u, i32 0, i32 1 to %z = getelementptr %struct.MYstr, %struct.MYstr* @mystr, i32 0, i32 1.

jdoerfert marked an inline comment as done.Thu, Dec 12, 12:14 PM
jdoerfert added inline comments.
llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
25

Sorry, I must have been blind.

The readonly on the function should not imply readonly on the byval argument because it is copied. Agreed. I'll fix that.

jdoerfert marked an inline comment as done.Thu, Dec 12, 6:23 PM
jdoerfert added inline comments.
llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
25

This should be fixed with rG6abd01e4624a2c9f8f76e11cc5d57cc7551b5d2a. I'll rerun the check scripts before I commit and verify the diff (if that is OK with you)

efriedma accepted this revision.Fri, Dec 13, 11:27 AM

LGTM

llvm/test/Transforms/Attributor/IPConstantProp/2009-09-24-byval-ptr.ll
25

Okay.

This revision is now accepted and ready to land.Fri, Dec 13, 11:27 AM
This revision was automatically updated to reflect the committed changes.