[PATCH] D48847: [GISel]: Add Legalization/lowering code for bit counting operations

[Roman Tereshin via Phabricator via llvm-commits]

rtereshin accepted this revision.
rtereshin added a comment.
This revision is now accepted and ready to land.

Thank you!

LGTM



================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:147
+  MachineRegisterInfo *MRI;
+};
+
----------------
aditya_nandakumar wrote:
> rtereshin wrote:
> > So we run multiple tests over the same instance of machine function, adding new instructions w/o clearing out the machine function and the rest of the context? Also, in some not really well defined order probably (like alphabetical by test name or something).
> > 
> > Shouldn't we put all of this into `setUp` instead and clear it out properly in `tearDown` and run every test in a clean state?
> I believe each googletest does not reuse the same test fixture object across multiple tests - it creates a new fixture object, constructs/calls setup, runs test, and call teardown()/destructor. I believe each test in it's own should be in a clean state when it runs.
> I hope I've not misunderstood your question.
True, (https://github.com/google/googletest/blob/master/googletest/docs/primer.md, https://github.com/google/googletest/blob/master/googletest/docs/faq.md#should-i-use-the-constructordestructor-of-the-test-fixture-or-the-set-uptear-down-function), maybe I got it mixed up with the python version of the library. Thanks!


================
Comment at: unittests/CodeGen/GlobalISel/LegalizerHelperTest.h:188
+  return FC.CheckInput(SM, OutBuffer, CheckStrings);
+}
----------------
aditya_nandakumar wrote:
> rtereshin wrote:
> > Do we run MachineVerifier somewhere here?
> No we don't currently run the verifier here.
Feels like a good idea to do that. Maybe, by default with the possibility to opt out on a test-case per test-case basis.


https://reviews.llvm.org/D48847