This is an archive of the discontinued LLVM Phabricator instance.

[FileCheck] Enforce --allow-unused-prefixes=false for llvm/test/Transforms
ClosedPublic

Authored by mtrofin on Dec 1 2020, 10:02 AM.

Details

Summary

Explicitly opt-out llvm/test/Transforms/Attributor.

Verified by flipping the default value of allow-unused-prefixes and
observing that none of the failures were under llvm/test/Transforms.

Diff Detail

Event Timeline

mtrofin created this revision.Dec 1 2020, 10:02 AM
Herald added a project: Restricted Project. · View Herald Transcript
mtrofin requested review of this revision.Dec 1 2020, 10:02 AM
jhenderson added inline comments.Dec 2 2020, 12:30 AM
llvm/test/Transforms/LoopIdiom/X86/left-shift-until-bittest.ll
38

ALL%s? This doesn't look right to me...

mtrofin updated this revision to Diff 308984.Dec 2 2020, 9:04 AM

fixed update_test_prefix.py and 2 tests

mtrofin marked an inline comment as done.
mtrofin added inline comments.
llvm/test/Transforms/LoopIdiom/X86/left-shift-until-bittest.ll
38

Thanks for the catch - added fix to update_test_prefix.py. What was happening was: that script tries to clean up cases where, after removing unused prefixes, we're left with one prefix. If the FileCheck was written with %s after the check-prefixes argument, it would consume the space between the prefix and %s, resulting in (for example) "--check-prefix=ALL%s". I fixed by hand a few of these cases, but didn't notice there could be a follow-up behavior: if the test had this "NOTE: Assertions have been autogenerated...", update_test_prefix would also re-run the autogenerator. In this case, that's update_test_checks.py, which would notice the new prefix (ALL%s), and insert checks for it.

The update to update_test_prefix.py is here for reference, I'll patch it separately, then rebase this change. Added @pengfei for that part, to make sure I haven't missed something.

Glad to see you are using the script. I'm not very sure its behavior. Please pay attention to its changes especially marked CHANGE

llvm/utils/update_test_prefix.py
22 ↗(On Diff #308984)

I think we just need to add a space for it:

s = re.sub('-?-check-prefixes=([^, ]+) ', '--check-prefix=\\1 ', s)

I once worried we may leave space at the end but didn't consider %s case.
\w+ is not always correct because many prefixes are using - in them.

mtrofin marked an inline comment as done.Dec 2 2020, 8:53 PM

Glad to see you are using the script. I'm not very sure its behavior. Please pay attention to its changes especially marked CHANGE

Yup, the tool is super-useful, thanks again!

Glad to see you are using the script. I'm not very sure its behavior. Please pay attention to its changes especially marked CHANGE

llvm/utils/update_test_prefix.py
22 ↗(On Diff #308984)

how about

s = re.sub('-?-check-prefixes=([\w_-]+)', '--check-prefix=\\1', s)

So it'd avoid the extra space, too? Or do we allow other characters?

Cheers~

llvm/utils/update_test_prefix.py
22 ↗(On Diff #308984)

I might tend to conservative. As far as I can see it, [\w-] should be Okay. But you need to check the last character is '\n' or ' '. Otherwise it will turn somthing -check-prefixes=A,B into -check-prefix=A,B. Maybe you can use below:

s = re.sub('-?-check-prefixes=([\w-]+\s)', '--check-prefix=\\1', s)
mtrofin marked an inline comment as done.Dec 2 2020, 9:31 PM
mtrofin added inline comments.
llvm/utils/update_test_prefix.py
22 ↗(On Diff #308984)

sg, I'll send a separate patch with that.

mtrofin updated this revision to Diff 309153.Dec 2 2020, 9:39 PM
mtrofin marked an inline comment as done.

separated change to update_test_prefix.py in D92542

jhenderson resigned from this revision.Dec 3 2020, 1:35 AM

I don't know anything about the Transform tests, so I don't feel qualified to further review whether what has been done is valid. Hopefully somebody else will be able to review.

Added a few more based on recent changes to relevant files - ptal. Thanks!

lebedev.ri resigned from this revision.Dec 3 2020, 12:18 PM

IMO this whole direction is wrong, but i'll guess i'll just have to suffer with new reality.

IMO this whole direction is wrong, but i'll guess i'll just have to suffer with new reality.

Not sure what direction you're referring to - if it's about handing unused prefixes, could you please contribute to the RFC thread (http://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html), it would be helpful to understand what tradeoffs we may have missed.

Thanks!

IMO this whole direction is wrong, but i'll guess i'll just have to suffer with new reality.

Not sure what direction you're referring to - if it's about handing unused prefixes,

Yep, precisely that direction.

could you please contribute to the RFC thread (http://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html), it would be helpful to understand what tradeoffs we may have missed.

Thanks!

I have not kept track of that RFC so i'm not sure if i'm just being picky because i'm not aware of a very good reasons pointed out there.

IMO this whole direction is wrong, but i'll guess i'll just have to suffer with new reality.

Not sure what direction you're referring to - if it's about handing unused prefixes,

Yep, precisely that direction.

could you please contribute to the RFC thread (http://lists.llvm.org/pipermail/llvm-dev/2020-October/146162.html), it would be helpful to understand what tradeoffs we may have missed.

Thanks!

I have not kept track of that RFC so i'm not sure if i'm just being picky because i'm not aware of a very good reasons pointed out there.

OK.

Any pushback re. this patch?

Thanks!

mtrofin updated this revision to Diff 309365.Dec 3 2020, 2:20 PM

leftover fix from python tool

gentle reminder - thanks!

Gentle reminder - @jdoerfert , is blank-allowing all the Transforms/Attributor to have unused prefixes "as intended"? (I remember you indicating that was the case)

I intend to commit this on Wednesday, it's basically NFC.

Thanks!

Gentle reminder - @jdoerfert , is blank-allowing all the Transforms/Attributor to have unused prefixes "as intended"? (I remember you indicating that was the case)

Yeah that's the case.

mtrofin updated this revision to Diff 310553.Dec 9 2020, 8:51 AM

added one more file that had unused prefixes

This revision was not accepted when it landed; it landed in state Needs Review.Dec 9 2020, 8:51 AM
This revision was landed with ongoing or failed builds.
This revision was automatically updated to reflect the committed changes.