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

Mon Apr 24 14:24:50 PDT 2023

dblaikie added a comment.

> 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.

I think the test is problematic in either form - either reaching into the source tree from the test tree, or reaching into the build tree from the test tree. But, yes, also I disagree with the substance of the test as well, regardless of the means it uses to achieve that.

> 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.

I generally disagree there - we should test the behavior of the compiler. So we should test how the behavior changes when the table changes.

If it's so boring (like we could test the features the table can describe, then all the tests that the features are used in the right way would be totally repetitive) as to be not worth testing that behavior - but then I don't see how testing the result of tblgen helps either.

Is it really non-obvious what the table does? (ie: when someone makes a change to the table, do they often make the wrong change to the table? And seeing the result of tblgen they would realize they'd made a mistake?)

> 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
> - 021b42367ab59f2e1d61d1b9624842e1d168d58d <https://reviews.llvm.org/rG021b42367ab59f2e1d61d1b9624842e1d168d58d>  Incorrect instruction names
> - e28376ec28b9034a35e01c95ccb4de9ccc6c4954 <https://reviews.llvm.org/rGe28376ec28b9034a35e01c95ccb4de9ccc6c4954> Missing optimization oppotunities

Could you explain the process in each case, in terms of how the test would've helped avoid the bug being introduced?

https://reviews.llvm.org/D147527#4249000 - @craig.topper - if you're around, perhaps you've got a take on this that'd help me wrap my head around it?

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147835