Page MenuHomePhabricator

[NFC][ArgPromo][Tests] Run update_test_checks on all ArgumentPromotion tests
AcceptedPublic

Authored by jdoerfert on Oct 10 2019, 12:15 AM.

Details

Summary

In preparation of D65531 as well as the reuse of these tests for the
Attributor, we modernize them and use the update_test_checks. Note that
there were some strugglers we will update the same way later on.

Event Timeline

jdoerfert created this revision.Oct 10 2019, 12:15 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 10 2019, 12:15 AM

I just run update on all of them and did not fix them yet. Please provide feedback if you have any, I'll update this revision asap.

Fix all but 3 tests, unknown what the problem there is

lebedev.ri retitled this revision from [ArgPromo][Tests] Run update_test_checks on all ArgumentPromotion tests to [NFC][ArgPromo][Tests] Run update_test_checks on all ArgumentPromotion tests.Oct 10 2019, 1:11 AM
lebedev.ri added inline comments.Oct 10 2019, 1:15 AM
llvm/test/Transforms/ArgumentPromotion/2008-02-01-ReturnAttrs.ll
1–3

Do you want:

; RUN: opt < %s -argpromotion -S | FileCheck %s --check-prefixes=ALL,ARGPROMOTION
; RUN: opt < %s -aa-pipeline='basic-aa' -passes=attributor -attributor-disable=false -attributor-max-iterations-verify -attributor-max-iterations=1 -S | FileCheck %s --check-prefixes=ALL,ATTRIBUTOR

?

llvm/test/Transforms/ArgumentPromotion/musttail.ll
48

does the script produce these verbose check-lines for define?
I think i usually saw it producing just the name.

jdoerfert marked 2 inline comments as done.Oct 10 2019, 9:43 AM

Thx for the initial feedback, I'll update this soon.

llvm/test/Transforms/ArgumentPromotion/2008-02-01-ReturnAttrs.ll
1–3

Will do

llvm/test/Transforms/ArgumentPromotion/musttail.ll
48

I took the original check lines where it made sense but I can also drop these in favor of the auto-generated ones, or I can allow the script to emit the arguments (which I think I preferred). I'll update the tests once I've done that.

Rerun update with newest version of D68819

If you want to add attributor runlines, i really insist on following the example i showed, it will result in cleaner diff overall.
That being said i like that the attributor runlines are in a separate diff.

jdoerfert updated this revision to Diff 224522.Oct 10 2019, 6:30 PM

Rerun with D68850 and updated D68819

If you want to add attributor runlines, i really insist on following the example i showed, it will result in cleaner diff overall.
That being said i like that the attributor runlines are in a separate diff.

D68852 adds the "privatizable pointer" attribute to the Attributor and runs it on all these test. I can split it so we first run it without the privatizable pointer attribute but it's unclear if that is helpful.

jdoerfert updated this revision to Diff 224795.Oct 13 2019, 2:40 PM

Rerun with fixed script to use argument variable names in body

Rerun with fixed script to use argument variable names in body

Can you please mark on which update_*.py patch which patch depends?

Use ALL,ARGPROMOTION as prefix list

If you want to add attributor runlines, i really insist on following the example i showed, it will result in cleaner diff overall.
That being said i like that the attributor runlines are in a separate diff.

I made the check prefixes ALL,ARGPROMOTION now and added the parent diffs (update_..._.py changes)

jdoerfert marked an inline comment as done.

run with ARGPROMOTION,ALL instead of ALL,ARGPROMOTION prefixes

lebedev.ri accepted this revision.Wed, Oct 30, 11:39 PM

If you want to add attributor runlines, i really insist on following the example i showed, it will result in cleaner diff overall.
That being said i like that the attributor runlines are in a separate diff.

I made the check prefixes ALL,ARGPROMOTION now and added the parent diffs (update_..._.py changes)

Thanks, LG now.

This revision is now accepted and ready to land.Wed, Oct 30, 11:39 PM

run with ARGPROMOTION,ALL instead of ALL,ARGPROMOTION prefixes

Uh, why, that's backwards? It will defeat the point of placing ALL first - then you won't have duplicate check lines for attributor.

run with ARGPROMOTION,ALL instead of ALL,ARGPROMOTION prefixes

Uh, why, that's backwards? It will defeat the point of placing ALL first - then you won't have duplicate check lines for attributor.

I think, determined by browsing the diff, that this way the next diff (with the Attributor) is smaller. If you think I should reverse it again, I can do that as well.

llvm/test/Transforms/ArgumentPromotion/musttail.ll
48

The "new" script (see parent revisions) does generate verbose check lines for defines and arguments. The former always the latter on request.

jdoerfert updated this revision to Diff 227331.Thu, Oct 31, 1:32 PM

Use improved scripts to update tests

jdoerfert updated this revision to Diff 227483.Fri, Nov 1, 10:49 AM

Rerun after D69701 to make the arguments part of the NOTE line

Harbormaster completed remote builds in B40408: Diff 227487.