This is an archive of the discontinued LLVM Phabricator instance.

[Utils] Check for more global information in update_test_checks
ClosedPublic

Authored by jdoerfert on Jan 14 2021, 7:20 PM.

Details

Summary

This allows to check for various globals (metadata/attributes/...) and
also resolves problems with globals (metadata/attributes/...) being
reused across different prefixes.

Diff Detail

Event Timeline

jdoerfert created this revision.Jan 14 2021, 7:20 PM
jdoerfert requested review of this revision.Jan 14 2021, 7:20 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 14 2021, 7:20 PM
mtrofin added inline comments.Feb 8 2021, 7:04 AM
llvm/utils/UpdateTestChecks/common.py
374–375

nit: could you run a linter to split, insomuch as possible, long lines? I (think I) know we don't enforce a code style of .py, but I'd argue there's readability value in doing it. (no need to scroll right, less eye movement effort)

jdoerfert added inline comments.Feb 9 2021, 4:04 PM
llvm/utils/UpdateTestChecks/common.py
374–375

I can, but FWIW, this isn't a complete patch yet. It does fix issues we have right now when global values, like #1 or !4, appear in a function but there was an issue with this when I run it on the Attributor tests.

And there are no tests :(

Unsure when I get the time to revisit this, anyone that thinks this is useful, pick up what you want.

kuter added a subscriber: kuter.Feb 28 2021, 6:33 PM

dict.keys() gives a dict_keys object which isn't a list. [:] doesn't work on it.

llvm/utils/UpdateTestChecks/common.py
601
739
jdoerfert updated this revision to Diff 327988.Mar 3 2021, 7:13 PM

Added tests, fixed issues, works with all attributor tests

jdoerfert retitled this revision from [Utils][WIP] Check for more global information in update_test_checks to [Utils] Check for more global information in update_test_checks.Mar 3 2021, 7:14 PM
jdoerfert edited the summary of this revision. (Show Details)
jdoerfert marked 3 inline comments as done.
jdoerfert added inline comments.
llvm/utils/UpdateTestChecks/common.py
374–375

If we define a linter I can run it. For now I broke a few long lines.

This revision is now accepted and ready to land.Mar 3 2021, 11:12 PM
This revision was landed with ongoing or failed builds.Mar 11 2021, 9:31 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptMar 11 2021, 9:31 PM
Herald added a subscriber: cfe-commits. · View Herald Transcript
nikic added a subscriber: nikic.Mar 13 2021, 2:10 PM

It looks like this has broken handling of getelementptr %T, and generates getelementptr [[T]] for it now, see e.g. the first change in https://github.com/llvm/llvm-project/commit/7ee96429a0b057bcc97331a6a762fc3cd00aed61.

In https://github.com/llvm/llvm-project/commit/6491e0165e96a93960e2dfb338c52c7eb155f408#diff-85c14e813467fc768fb641be9567780053ef1162da8cc12dd6bcb29d5e14384eR575 I was forced to rename the function argument to avoid a clash between the type %S2 and the value %s2.

It looks like this has broken handling of getelementptr %T, and generates getelementptr [[T]] for it now, see e.g. the first change in https://github.com/llvm/llvm-project/commit/7ee96429a0b057bcc97331a6a762fc3cd00aed61.

I don't see the problem:
Source is ...(%ABC, %ABC, ...
Before we checked: ...(%ABC, [[ABC:%.*]], ...
Now we check: ...([[ABC:%.*]], [[ABC]], ...
That seems strictly better to me. What am I missing?

In https://github.com/llvm/llvm-project/commit/6491e0165e96a93960e2dfb338c52c7eb155f408#diff-85c14e813467fc768fb641be9567780053ef1162da8cc12dd6bcb29d5e14384eR575 I was forced to rename the function argument to avoid a clash between the type %S2 and the value %s2.

Hm, this one I can see. We could work around that by looking if there is a global with the same name and then add a _lcl suffix.
I can try to look into it.