[PATCH] D155734: [X86][AArch64] Add additional extract_lowbits test

Wed Jul 19 14:33:07 PDT 2023

goldstein.w.n added a comment.

In D155734#4516276 <https://reviews.llvm.org/D155734#4516276>, @danilaml wrote:

> In D155734#4516168 <https://reviews.llvm.org/D155734#4516168>, @goldstein.w.n wrote:
>
>> 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 <https://reviews.llvm.org/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!

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D155734/new/

https://reviews.llvm.org/D155734