Daniel Sanders via llvm-dev
2018-Jul-03 18:44 UTC
[llvm-dev] Using FileCheck in unit tests
> On 2 Jul 2018, at 15:13, Matthias Braun via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > I had similar gripes with unit testing machine function stuff. I personally would have preferred to create more tests based on a tool like llc rather than pushing more on the unit test side. Anyway I tried to push https://reviews.llvm.org/D48850 <https://reviews.llvm.org/D48850> in order to extend llc to be more amenable in the situations we want to test, but it ultimately wasn't accepted. > > - Matthias > >> On Jul 2, 2018, at 2:35 PM, Aditya Nandakumar via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >> >> When writing MachineFunction unit tests, I find it quite tedious to verify correctness in C++. I would like to use FileCheck in UnitTests because FileCheck is extremely convenient/robust to verify correctness. In order to do so, I moved most of FileCheck’s implementation into a header (Support/FileCheck.h) and updated FileCheck.cpp to use this. >> >> I ran into this while writing some target agnostic Legalization code in GISel where it’s not possible or extremely inconvenient to use llc.To elaborate on this, the motivating issue as I understand it is that there's portions of GlobalISel that will be needed by targets in the future but aren't used by the current set of targets that support GlobalISel. For example, of the various actions (widenScalar, narrowScalar, Lower, etc.) that can be taken on CTLZ, most of the current GlobalISel targets declare CTLZ legal for almost all the types they encounter in practice. Instead of leaving the other paths untested or unimplemented, you wanted to be able to test those parts of GlobalISel even though we lack a target that would allow us to test it with llc. Is that right?>> What do others think of this? I would think there might be non GISel uses for FileCheck in unit tests, but I’m not sure.I like the idea of being able to build a mock target to test the areas of GlobalISel that would normally only get very light testing. Being able to test the individual expansions in the combiner is going to be particularly useful as there's several reasons an individual combine could bitrot over time such there being other combines that overlap many of the same cases, or just simply being a rare case. I'm not keen on moving the implementation to a header though. If we're going to make it usable from unittests then I think we should turn it into a library and compile the implementation once.>> Here’s a quick patch for this (https://reviews.llvm.org/D48850 <https://reviews.llvm.org/D48850>) >> >> Thanks >> Aditya >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180703/5b5d73e9/attachment.html>
Aditya Nandakumar via llvm-dev
2018-Jul-03 18:48 UTC
[llvm-dev] Using FileCheck in unit tests
> On Jul 3, 2018, at 11:44 AM, Daniel Sanders <daniel_l_sanders at apple.com> wrote: > > > >> On 2 Jul 2018, at 15:13, Matthias Braun via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >> >> I had similar gripes with unit testing machine function stuff. I personally would have preferred to create more tests based on a tool like llc rather than pushing more on the unit test side. Anyway I tried to push https://reviews.llvm.org/D48850 <https://reviews.llvm.org/D48850> in order to extend llc to be more amenable in the situations we want to test, but it ultimately wasn't accepted. >> >> - Matthias >> >>> On Jul 2, 2018, at 2:35 PM, Aditya Nandakumar via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >>> >>> When writing MachineFunction unit tests, I find it quite tedious to verify correctness in C++. I would like to use FileCheck in UnitTests because FileCheck is extremely convenient/robust to verify correctness. In order to do so, I moved most of FileCheck’s implementation into a header (Support/FileCheck.h) and updated FileCheck.cpp to use this. >>> >>> I ran into this while writing some target agnostic Legalization code in GISel where it’s not possible or extremely inconvenient to use llc. > > To elaborate on this, the motivating issue as I understand it is that there's portions of GlobalISel that will be needed by targets in the future but aren't used by the current set of targets that support GlobalISel. For example, of the various actions (widenScalar, narrowScalar, Lower, etc.) that can be taken on CTLZ, most of the current GlobalISel targets declare CTLZ legal for almost all the types they encounter in practice. Instead of leaving the other paths untested or unimplemented, you wanted to be able to test those parts of GlobalISel even though we lack a target that would allow us to test it with llc.Yup. That was the main issue. There are also cases where expansion of one instruction changes if some other opcode is legal/custom etc and there’s no way to test both paths/expansions for one target.> > Is that right? > >>> What do others think of this? I would think there might be non GISel uses for FileCheck in unit tests, but I’m not sure. > > I like the idea of being able to build a mock target to test the areas of GlobalISel that would normally only get very light testing. Being able to test the individual expansions in the combiner is going to be particularly useful as there’s several reasons an individual combine could bitrot over time such there being other combines that overlap many of the same cases, or just simply being a rare case.Yup. I think it would make testing combines/transformations much easier.> > I'm not keen on moving the implementation to a header though. If we’re going to make it usable from unittests then I think we should turn it into a library and compile the implementation once.I can definitely do that - this was a quick POC. Thanks> >>> Here’s a quick patch for this (https://reviews.llvm.org/D48850 <https://reviews.llvm.org/D48850>) >>> >>> Thanks >>> Aditya >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> >> >> _______________________________________________ >> LLVM Developers mailing list >> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev >-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180703/4f7f0870/attachment.html>
> On Jul 3, 2018, at 11:48 AM, Aditya Nandakumar via llvm-dev <llvm-dev at lists.llvm.org> wrote: > > > >> On Jul 3, 2018, at 11:44 AM, Daniel Sanders <daniel_l_sanders at apple.com <mailto:daniel_l_sanders at apple.com>> wrote: >> >> >> >>> On 2 Jul 2018, at 15:13, Matthias Braun via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >>> >>> I had similar gripes with unit testing machine function stuff. I personally would have preferred to create more tests based on a tool like llc rather than pushing more on the unit test side. Anyway I tried to push https://reviews.llvm.org/D48850 <https://reviews.llvm.org/D48850> in order to extend llc to be more amenable in the situations we want to test, but it ultimately wasn't accepted. >>> >>> - Matthias >>> >>>> On Jul 2, 2018, at 2:35 PM, Aditya Nandakumar via llvm-dev <llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org>> wrote: >>>> >>>> When writing MachineFunction unit tests, I find it quite tedious to verify correctness in C++. I would like to use FileCheck in UnitTests because FileCheck is extremely convenient/robust to verify correctness. In order to do so, I moved most of FileCheck’s implementation into a header (Support/FileCheck.h) and updated FileCheck.cpp to use this. >>>> >>>> I ran into this while writing some target agnostic Legalization code in GISel where it’s not possible or extremely inconvenient to use llc. >> >> To elaborate on this, the motivating issue as I understand it is that there's portions of GlobalISel that will be needed by targets in the future but aren't used by the current set of targets that support GlobalISel. For example, of the various actions (widenScalar, narrowScalar, Lower, etc.) that can be taken on CTLZ, most of the current GlobalISel targets declare CTLZ legal for almost all the types they encounter in practice. Instead of leaving the other paths untested or unimplemented, you wanted to be able to test those parts of GlobalISel even though we lack a target that would allow us to test it with llc. > Yup. That was the main issue. There are also cases where expansion of one instruction changes if some other opcode is legal/custom etc and there’s no way to test both paths/expansions for one target. >> >> Is that right? >> >>>> What do others think of this? I would think there might be non GISel uses for FileCheck in unit tests, but I’m not sure. >> >> I like the idea of being able to build a mock target to test the areas of GlobalISel that would normally only get very light testing. Being able to test the individual expansions in the combiner is going to be particularly useful as there’s several reasons an individual combine could bitrot over time such there being other combines that overlap many of the same cases, or just simply being a rare case. > Yup. I think it would make testing combines/transformations much easier.+ 1, I can think of a handful of tests in unittests/Transforms/Utils which could be simplified using FileCheck-style checks. vedant>> >> I'm not keen on moving the implementation to a header though. If we’re going to make it usable from unittests then I think we should turn it into a library and compile the implementation once. > I can definitely do that - this was a quick POC. > Thanks >> >>>> Here’s a quick patch for this (https://reviews.llvm.org/D48850 <https://reviews.llvm.org/D48850>) >>>> >>>> Thanks >>>> Aditya >>>> >>>> _______________________________________________ >>>> LLVM Developers mailing list >>>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >>>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> >>> >>> _______________________________________________ >>> LLVM Developers mailing list >>> llvm-dev at lists.llvm.org <mailto:llvm-dev at lists.llvm.org> >>> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev <http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev> >> > > _______________________________________________ > LLVM Developers mailing list > llvm-dev at lists.llvm.org > http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev-------------- next part -------------- An HTML attachment was scrubbed... URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20180709/4a9db11a/attachment.html>