[llvm-dev] Using FileCheck in unit tests

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

[llvm-dev] Using FileCheck in unit tests

Robin Eklind via llvm-dev
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.
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.

Here’s a quick patch for this (https://reviews.llvm.org/D48850)

Thanks
Aditya


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Using FileCheck in unit tests

Robin Eklind via llvm-dev
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 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 <[hidden email]> 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.
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.

Here’s a quick patch for this (https://reviews.llvm.org/D48850)

Thanks
Aditya

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Using FileCheck in unit tests

Robin Eklind via llvm-dev


On 2 Jul 2018, at 15:13, Matthias Braun via llvm-dev <[hidden email]> 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 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 <[hidden email]> 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)

Thanks
Aditya

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Using FileCheck in unit tests

Robin Eklind via llvm-dev


On Jul 3, 2018, at 11:44 AM, Daniel Sanders <[hidden email]> wrote:



On 2 Jul 2018, at 15:13, Matthias Braun via llvm-dev <[hidden email]> 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 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 <[hidden email]> 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)

Thanks
Aditya

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev



_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Using FileCheck in unit tests

Robin Eklind via llvm-dev


On Jul 3, 2018, at 11:48 AM, Aditya Nandakumar via llvm-dev <[hidden email]> wrote:



On Jul 3, 2018, at 11:44 AM, Daniel Sanders <[hidden email]> wrote:



On 2 Jul 2018, at 15:13, Matthias Braun via llvm-dev <[hidden email]> 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 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 <[hidden email]> 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)

Thanks
Aditya

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev


_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
Reply | Threaded
Open this post in threaded view
|

Re: [llvm-dev] Using FileCheck in unit tests

Robin Eklind via llvm-dev
In reply to this post by Robin Eklind via llvm-dev
Aditya Nandakumar via llvm-dev <[hidden email]> writes:

> 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.
> 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.
>
> Here’s a quick patch for this (https://reviews.llvm.org/D48850
> <https://reviews.llvm.org/D48850>)

Vedant Kumar via llvm-dev <[hidden email]> writes:
> + 1, I can think of a handful of tests in unittests/Transforms/Utils
> which could be simplified using FileCheck-style checks.

Sounds like there's some general consensus that this is useful, so I'll
go ahead and start reviewing the patch.

Matthias Braun via llvm-dev <[hidden email]> writes:
> 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 in order to extend llc
> to be more amenable in the situations we want to test, but it
> ultimately wasn't accepted.

Looks like you put the wrong link here and I'm having trouble finding
the review you're referring to. Do you mind digging it up for me?

_______________________________________________
LLVM Developers mailing list
[hidden email]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev