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

Mon Apr 24 16:17:47 PDT 2023

dblaikie added a comment.

In D147835#4293973 <https://reviews.llvm.org/D147835#4293973>, @craig.topper wrote:

> In D147835#4293739 <https://reviews.llvm.org/D147835#4293739>, @dblaikie wrote:
>
>>> 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?
>
> The majority of X86 instructions have a register only form and a form that has a memory address operand to load from. Or sometimes store to. Each of the two form is represented separately in the `X86::` opcode space and is a separate record in tablegen.
>
> The tables here pair up the register only form and the memory form of the instructions into a single row in the table. This information is used to convert between the two forms. For example, to fold a stack reload created by register allocation into its using instruction. Or to unfold a load so it can be hoisted by MachineLICM.
>
> There is no direct connection between the two instruction records in the tablegen definitions. So the tablegen emitter has to compare the opcodes, format field, and other encoding information from the tablegen records to find the matching instructions to pair up.
>
> Unfortunately, just because two instructions are encoded in very similar way does not mean they are really two different forms of the same operation. This has gotten worse as the opcode map has gotten more crowed. Intel and AMD have gone back and filled in new instructions near the opposite form of an older instruction. It would not be legal to convert between these. There are rules in the tablegen emitter or tablegen records to block some of these false cases. It may be possible to detect this by doing some string matching of the names if we came up with a reliable way to extract the instruction name from the form suffix we had to describe the operands. For example ADD32rr and ADD32rm are paired up and the "rm" and "rr" suffix indicate one is memory and one is register.
>
> If two instructions are the same instruction, it doesn't mean it is always safe to fold or unfold. Sometimes only one direction is ok or sometimes neither directions is ok. The tablegen emitter has rules for the known blocking reasons, but new blocking reasons could be created by future instructions.
>
> As I understand it, the idea here is that every time a new instruction is added this test will fail and the person adding a new instruction will have to audit the newly generated table entries for correctness. The test won't fail if the newly added instructions hit one of the existing black list rules. Usually the new entries will be fine and they can be copied over to the golden file and be done. Less often a new restriction will need to be added to prevent tablegen from generating an incorrect entry in the table. Relatively few people are adding new instructions so most developers will never see this test fail.
>
> Previously this audit happened only occasionally when someone manually ran the tablegen emitter and checked for entries that weren't in the golden file. This test makes this audit occur when the instructions get added instead of some unknown point in the future.
>
> We don't know how to make a future proof tablegen emitter so we want to have a human audit in the future. Unfortunately, the human audit can also be wrong, but hopefully other humans will also audit as part of the code review when adding the new instructions.

Thanks for the context - I don't suppose it's worthwhile to make these associations explicit (you mentioned " There is no direct connection between the two instruction records in the tablegen definitions.") so there weren't these unreliable heuristics to contend with?

In any case - the test is still a bit of a layering violation - it's not a test of tblgen itself, I think (lib/TableGen only depends on Support, not on Target, etc)? This test is really a test of the X86.td file, by the sounds of it (well, maybe that and the TableGen backend here - which I guess has all the special case rules encoded?) - perhaps the test should live in Target/X86 instead? (though I don't think it'd fix Google's internal layering issues - of having a lit test reading a source file - but would at least feel more right to me - in terms of library dependency graph, at least)

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D147835