This is an archive of the discontinued LLVM Phabricator instance.

[X86][update_llc_test_checks] Use a less greedy regular expression for replacing constant pool labels in tests.
ClosedPublic

Authored by craig.topper on Mar 27 2021, 4:47 PM.

Details

Summary

While working on D97208 I noticed that these greedy regular
expressions prevent tests from failing when (%rip) appears after
a constant pool label when it didn't before.

Diff Detail

Unit TestsFailed

Event Timeline

craig.topper created this revision.Mar 27 2021, 4:47 PM
craig.topper requested review of this revision.Mar 27 2021, 4:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 27 2021, 4:47 PM

Are these changed manually? Can we update them with --no_x86_scrub_rip, i.e.

llvm/utils/update_llc_test_checks.py llvm/test/CodeGen/X86/WidenArith.ll --no_x86_scrub_rip

Are these changed manually? Can we update them with --no_x86_scrub_rip, i.e.

llvm/utils/update_llc_test_checks.py llvm/test/CodeGen/X86/WidenArith.ll --no_x86_scrub_rip

Oh, I saw the change on script. LGTM~

Are these changed manually? Can we update them with --no_x86_scrub_rip, i.e.

llvm/utils/update_llc_test_checks.py llvm/test/CodeGen/X86/WidenArith.ll --no_x86_scrub_rip

I modified asm.py and made it print this now. Most of the affected tests are 32-bit tests that don't use %rip so -no_x86_scrub_rip wouldn't affect them.

For 64-bit tests scrubbing rip replaces any tests before (%rip) with a regular expression like {{.*}}(%rip). The test check line will always contain %rip if it is part of the assembly. This matches before the LCP match. So for most 64-bit tests the presence of %rip prevents the LCP from being replaced with {{\.LCPI.*}}.

For cases affected by D97208, %rip is not currently present so the LCP scrub kicks in. Producing {{\.LCPI.*}} followed by a comma. Because that regex is greedy it will match up to the next comma. So whether there is an %rip in asm or not the regex will alway match. If you apply the X86InstrInfo.cpp and run the script on avx-cmp.ll and mmx-fold-zero.ll you'll get

{{.*}}(%rip)

If you add -no_x86_scrub_rip you'll get

{{LCPI.*}}(%rip)

Are these changed manually? Can we update them with --no_x86_scrub_rip, i.e.

llvm/utils/update_llc_test_checks.py llvm/test/CodeGen/X86/WidenArith.ll --no_x86_scrub_rip

I modified asm.py and made it print this now. Most of the affected tests are 32-bit tests that don't use %rip so -no_x86_scrub_rip wouldn't affect them.

For 64-bit tests scrubbing rip replaces any tests before (%rip) with a regular expression like {{.*}}(%rip). The test check line will always contain %rip if it is part of the assembly. This matches before the LCP match. So for most 64-bit tests the presence of %rip prevents the LCP from being replaced with {{\.LCPI.*}}.

For cases affected by D97208, %rip is not currently present so the LCP scrub kicks in. Producing {{\.LCPI.*}} followed by a comma. Because that regex is greedy it will match up to the next comma. So whether there is an %rip in asm or not the regex will alway match. If you apply the X86InstrInfo.cpp and run the script on avx-cmp.ll and mmx-fold-zero.ll you'll get

{{.*}}(%rip)

If you add -no_x86_scrub_rip you'll get

{{LCPI.*}}(%rip)

I see, thanks.

llvm/test/CodeGen/X86/WidenArith.ll
14

Nit: Should this be simplified to [0-9_]+?

craig.topper added inline comments.Mar 27 2021, 10:43 PM
llvm/test/CodeGen/X86/WidenArith.ll
14

We could but I thought it was best to keep it in sync with the scrub regex

SCRUB_X86_LCP_RE = re.compile(r'\.LCPI[0-9]+_[0-9]+')
pengfei accepted this revision.Mar 28 2021, 1:48 AM

LGTM.

llvm/test/CodeGen/X86/WidenArith.ll
14

Agreed.

This revision is now accepted and ready to land.Mar 28 2021, 1:48 AM
RKSimon accepted this revision.Mar 28 2021, 3:47 AM

LGTM

This revision was landed with ongoing or failed builds.Mar 28 2021, 11:40 AM
This revision was automatically updated to reflect the committed changes.