[PATCH] D147835: [X86][mem-fold] Speed up test by not re-generating the .inc file

Wed Apr 19 02:14:53 PDT 2023

skan added a comment.

In D147835#4278727 <https://reviews.llvm.org/D147835#4278727>, @dblaikie wrote:

> In D147835#4252818 <https://reviews.llvm.org/D147835#4252818>, @skan wrote:
>
>> In D147835#4252780 <https://reviews.llvm.org/D147835#4252780>, @dblaikie wrote:
>>
>>> Could you describe in more detail the purpose/value of this test? Judging by what I can see here it looks like it's a golden file test that verifies a copy of a file matches the current one - this doesn't seem valuable/seems like busy-work to keep an extra copy of a file around?
>>
>> Sure, let me explain in more detail. `X86GenFoldTables.inc` consists of tables for memory folding and is used in some optimizations like RA, LICM, which is generated by a tblgen backend "-gen-x86-fold-tables". The .inc file is not in source code of LLVM and is produced in the build process. `x86-fold-tables.inc` is the only source file that contains the tables, so there is no “extra copy" here.
>>
>> We define some common rules in `X86FoldTablesEmitter.cpp` for the tblgen backend and these rules apply to most of existing and future instructions. But you know, there are always exceptions and a future instruction may not comply with these rules, causing some entries in `X86GenFoldTables.inc` to be incorrect. We need a way a to detect it, this is the first reason this test exists.
>>
>> Before D147527 <https://reviews.llvm.org/D147527> and we had this test, we expected developers to generate the `.inc` file in their local workspace and update hand-written tables biweekly or monthly. Unfortunately, people may forget it and tend to not fix the rules in `X86FoldTablesEmitter.cpp` when they were broken.  The longer we forget, the bigger the diff we need to review. The same wrong entries appeared in front of the reviewer again and again b/c no one fixed the broken rules.
>>
>> If the developer can update the table or correct the broken rules each time a small set of instructions are added, the problem can be solved.  This is the second reason this test exists.
>
> I still don't really follow. This is a golden file test that boils down to "if this file changes, go check the other file/make updates to it" by checking in the built-result into x86-fold-tables.inc and comparing against the newly built result...
>
> This doesn't seem suitable as a test in general, even without this quirky "test that reads a file from the build directory/depends on the build directory layout".
>
> Could you point to some examples of problems this test (or the previous version, or the problems that arose in the absence of this test) helps identify? Perhaps there are other ways to address them.

This patch was already reverted. The build directory layout is not an issue anymore and I think we only need to discuss about why we need the "golden" file test.

First, I think it's suitable as a test in general. Tablgen generates the tables for compiler here and then compiler optimizes based on the tables. Excluding the tables, the optimization is well tested. So what we need to check is the tables. From this perspective, the test checks the functionality of the compiler.
Second, there are some examples where this test could have helped identify. (I searched them by `git log --follow f3d9abf1f87c~ -- llvm/lib/Target/X86/X86MemFoldTables.inc`, `git log --follow 4f4b2161ec3c~ -- llvm/lib/Target/X86/X86InstrFoldTables.cpp `)

- 7265486208d7a86c4751812e685e8d88e3c24291 <https://reviews.llvm.org/rG7265486208d7a86c4751812e685e8d88e3c24291> Missing/incorrect items in tables
- 4d2149700618553eb28a8bc6b74664e4ba6aca65 <https://reviews.llvm.org/rG4d2149700618553eb28a8bc6b74664e4ba6aca65> Incorrect attributes
- 41052fd699fcbc10340e8983d94d92f32c4bd547 <https://reviews.llvm.org/rG41052fd699fcbc10340e8983d94d92f32c4bd547> Copy-paste error for instruction names
- e28376ec28b9034a35e01c95ccb4de9ccc6c4954 <https://reviews.llvm.org/rGe28376ec28b9034a35e01c95ccb4de9ccc6c4954> Missing optimization oppotunities

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147835