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

David Blaikie via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Apr 18 15:04:17 PDT 2023

dblaikie added a comment.

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.

  rG LLVM Github Monorepo



More information about the llvm-commits mailing list