Page MenuHomePhabricator

[UpdateTestChecks] Default --x86_scrub_rip to False
ClosedPublic

Authored by MaskRay on May 16 2021, 3:28 PM.

Details

Summary

True is a bad default: the useful symbol names and @GOTPCREL are scrubbed.

Change the default and add global variable tests to x86-basic.ll
(renamed from x86_function_name.ll since we now also test variables).
I updated some tests to show the differences.

Updated LCPI regex to include Darwin style LCPI_[0-9]+_[0-9]+ (no
leading dot).

Diff Detail

Event Timeline

MaskRay created this revision.May 16 2021, 3:28 PM
MaskRay requested review of this revision.May 16 2021, 3:28 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 16 2021, 3:28 PM
MaskRay updated this revision to Diff 345732.May 16 2021, 3:36 PM

fix tests

The switch to --no-x86_scrub_rip by default makes sense to me.

You've not enabled --x86_scrub_rip for any test as far as I see, right? Do we know what tests the option is useful for? If so, perhaps their update_llc_test_checks invocations could be updated already.

The switch to --no-x86_scrub_rip by default makes sense to me.

You've not enabled --x86_scrub_rip for any test as far as I see, right? Do we know what tests the option is useful for? If so, perhaps their update_llc_test_checks invocations could be updated already.

It's the default. The updated tests have revealed the differences.

Updated LCPI regex to include Darwin style LCPI_[0-9]+_[0-9]+ (no
leading dot).

Do we have a test for it?

llvm/test/CodeGen/X86/abi-isel.ll
1615–1616

Not sure how many testing scenarios like this. But I think this is the origin of this knob. Can we change it to something like
SCRUB_X86_RIP_RE = re.compile(r'[.\d]+\(%rip\)') instead?

hvdijk added inline comments.May 21 2021, 11:46 AM
llvm/test/CodeGen/X86/abi-isel.ll
1615–1616

The LINUX-32-STATIC case already tested for movl dsrc+64, %eax. It seems a bit silly to me to allow a fixed offset there but to not allow it in the (%rip) case, so I personally do like the test being updated this way.

Updated LCPI regex to include Darwin style LCPI_[0-9]+_[0-9]+ (no
leading dot).

Do we have a test for it?

llvm/test/CodeGen/X86/WidenArith.ll

MaskRay marked 2 inline comments as done.May 21 2021, 12:31 PM
MaskRay added inline comments.
llvm/test/CodeGen/X86/abi-isel.ll
1615–1616

I think it is important to test the offset.

This is not an abitrary offset in a linked image.

pengfei accepted this revision.May 21 2021, 7:18 PM

Updated LCPI regex to include Darwin style LCPI_[0-9]+_[0-9]+ (no
leading dot).

Do we have a test for it?

llvm/test/CodeGen/X86/WidenArith.ll

It seems it still has leading dot. Anyway, just nitpick.

This revision is now accepted and ready to land.May 21 2021, 7:18 PM
MaskRay updated this revision to Diff 347166.May 21 2021, 7:20 PM
MaskRay marked an inline comment as done.

Add x86-constant-tool.test which I forgot in the previous revision

Updated LCPI regex to include Darwin style LCPI_[0-9]+_[0-9]+ (no
leading dot).

Do we have a test for it?

llvm/test/CodeGen/X86/WidenArith.ll

It seems it still has leading dot. Anyway, just nitpick.

See llvm/test/tools/UpdateTestChecks/update_llc_test_checks/Inputs/x86-constant-pool.ll.expected. Darwin's temporary symbols start with L instead of .L

This revision was landed with ongoing or failed builds.May 21 2021, 7:26 PM
This revision was automatically updated to reflect the committed changes.