This is an archive of the discontinued LLVM Phabricator instance.

[X86][AArch64] Add additional extract_lowbits test
ClosedPublic

Authored by danilaml on Jul 19 2023, 11:02 AM.

Details

Summary

Check that vreg_width-1 mask is only removed for shifts

Diff Detail

Event Timeline

danilaml created this revision.Jul 19 2023, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 11:02 AM
danilaml requested review of this revision.Jul 19 2023, 11:02 AM
Herald added a project: Restricted Project. · View Herald TranscriptJul 19 2023, 11:02 AM

Your change is x86 only. Why are you adding aarch64 tests?

Also you have the order flipped around. The test commit should be the child of the implementation, not the other way around.

Your change is x86 only. Why are you adding aarch64 tests?

My change is x86 only, but the tests are not. These tests were added on your suggestion that the coverage for masked nbits was lacking, which is not related to the change I was making. And the test file explicitly asks to keep the tests in sync between x86 and aarch64. Do you see any benefit in splitting it further?

Also you have the order flipped around. The test commit should be the child of the implementation, not the other way around.

Is it not? I don't understand then how this works. Phabricator states that D155622 was added as a parent, doesn't that make this (test) commit its child?

Your change is x86 only. Why are you adding aarch64 tests?

My change is x86 only, but the tests are not. These tests were added on your suggestion that the coverage for masked nbits was lacking, which is not related to the change I was making. And the test file explicitly asks to keep the tests in sync between x86 and aarch64. Do you see any benefit in splitting it further?

Okay, but then add an aarch64 maintainer on the review list (also add phoebe on this one).

Also you have the order flipped around. The test commit should be the child of the implementation, not the other way around.

Is it not? I don't understand then how this works. Phabricator states that D155622 was added as a parent, doesn't that make this (test) commit its child?

Crap, you are right. Make the tests the parent. Sorry!

Okay, but then add an aarch64 maintainer on the review list (also add phoebe on this one).

Added more reviewers but the changes are rather NFC-ish. Just some asm added annotations and the test that should compile to the same assembly as the preceding one.

pengfei accepted this revision.Jul 19 2023, 6:25 PM

LGTM. If possible, you could add new test on the end of the file, so that you won't get a lot of BB number changes.

This revision is now accepted and ready to land.Jul 19 2023, 6:25 PM

LGTM. If possible, you could add new test on the end of the file, so that you won't get a lot of BB number changes.

I just the felt that it logically fits better right after the test it was based on (and it's easier to confirm that the asm doesn't change for non-bzhi case that way). I feel like update_llc_test_checks should be able to do something with labels, since their name is usually not important, but it doesn't seem like it does currently.

This revision was landed with ongoing or failed builds.Jul 19 2023, 11:18 PM
This revision was automatically updated to reflect the committed changes.