This is an archive of the discontinued LLVM Phabricator instance.

[X86][TableGen] Recommitting the X86 memory folding tables TableGen backend while disabling it by default.
ClosedPublic

Authored by aymanmus on Sep 19 2017, 1:39 AM.

Details

Summary

After the original commit (rL304088) was reverted, a discussion in llvm-dev was opened on 'how to accomplish this task'.
In the discussion we concluded that the best way to achieve our goal (which is to automate the folding tables and remove the manually maintained tables) is:

  1. Commit the tablegen backend disabled by default.
  2. Proceed with an incremental updating of the manual tables - while checking the validity of each added entry.
  3. Repeat previous step until we reach a state where the generated and the manual tables are identical. Then we can safely remove the manual tables and include the generated tables instead.
  4. Schedule periodical (1 week/2 weeks/1 month) runs of the pass:
    • if changes appear (new entries):
      • make sure the entries are legal
      • If they are not, mark them as illegal to folding
    • Commit the changes (if there are any).

CMake flag added for this purpose is "X86_GEN_FOLD_TABLES". Building with this flags will run the pass and emit the X86GenFoldTables.inc file under build/lib/Target/X86/ directory which is a good reference for any developer who wants to take part in the effort of completing the current folding tables.

Diff Detail

Event Timeline

aymanmus created this revision.Sep 19 2017, 1:39 AM
craig.topper edited edge metadata.Sep 19 2017, 10:55 AM

Why can't we just generate the table by default and not include it yet? Then anyone can look at the table at any time without having to do something extra?

Why can't we just generate the table by default and not include it yet? Then anyone can look at the table at any time without having to do something extra?

Since the backend would be disabled at the end of the process, I think that it's better to keep it that way from the beginning.
The generated tables do not mean anything now and IMHO should not be visible to anyone who has no interest in contributing to this specific effort.
So I guess the best way to keep the temporary tables "hidden" is by enabling the "run-by-demand" mechanism now.

While will the backend be disabled at the end of the process? Won't be using the backend in place of the manual tables?

No it won't completely replace it. It would be used as a reference to the manual tables.
It's explained in the description section of this patch.
The decision was made after a discussion in the community's mailing list (http://lists.llvm.org/pipermail/llvm-dev/2017-July/115734.html)

Bullet 3 in this patch says “remove the manual tables” and “include the generated tables”.

@craig.topper that's right, at the end of this process both the generated and manual tables will hold the same entries. But the backend would be disabled and we would include a copy of the last generated inc file from it.
The main challenge here is how to integrate the TableGen's backend and taking advantage of its capabilities while supervising and validating each change made to them.

This revision is now accepted and ready to land.Oct 4 2017, 4:13 PM
This revision was automatically updated to reflect the committed changes.
thakis added a subscriber: thakis.Dec 8 2018, 1:24 PM

It's been a bit over a year, what's the status here? lib/Target/X86/CMakeLists.txt still has a check for X86_GEN_FOLD_TABLES but nothing sets that from what I can tell.

You're correct, nothing sets it. I'm nervous about ever using the autogenerated table by default. I've used it a while back to copy entries into the manual tables. But I had to add more waivers and adjustments to fix bugs in the autogenerated table in the process. I don't think they are still a perfect match. I worry that if we switch to the autogenerated table and as we add new instructions in the future we can silently introduce bugs because the new table entries weren't reviewed.

thakis added a comment.Dec 8 2018, 7:34 PM

You're correct, nothing sets it. I'm nervous about ever using the autogenerated table by default. I've used it a while back to copy entries into the manual tables. But I had to add more waivers and adjustments to fix bugs in the autogenerated table in the process. I don't think they are still a perfect match. I worry that if we switch to the autogenerated table and as we add new instructions in the future we can silently introduce bugs because the new table entries weren't reviewed.

Should we remove the autogenerated table again then?

You're correct, nothing sets it. I'm nervous about ever using the autogenerated table by default. I've used it a while back to copy entries into the manual tables. But I had to add more waivers and adjustments to fix bugs in the autogenerated table in the process. I don't think they are still a perfect match. I worry that if we switch to the autogenerated table and as we add new instructions in the future we can silently introduce bugs because the new table entries weren't reviewed.

Should we remove the autogenerated table again then?

It still has merit as a debugging tool to check against the manually created tables - not sure if that's a big enough reason?

thakis added a comment.Dec 9 2018, 6:01 AM

You're correct, nothing sets it. I'm nervous about ever using the autogenerated table by default. I've used it a while back to copy entries into the manual tables. But I had to add more waivers and adjustments to fix bugs in the autogenerated table in the process. I don't think they are still a perfect match. I worry that if we switch to the autogenerated table and as we add new instructions in the future we can silently introduce bugs because the new table entries weren't reviewed.

Should we remove the autogenerated table again then?

It still has merit as a debugging tool to check against the manually created tables - not sure if that's a big enough reason?

Up to the folks using it :-) Out of curiosity, when you do this checking, do you manually run tablegen, or do you set this cmake var somehow?

Up to the folks using it :-) Out of curiosity, when you do this checking, do you manually run tablegen, or do you set this cmake var somehow?

Looks like just passing -DX86_GEN_FOLD_TABLES=ON to cmake is enough, so I'm guessing that's what you do.

But if y'all basically invoke this manually and then copy bits over, maybe you run llvm-tblgen -gen-x86-fold-tables directly? In that case, we could remove the (to me confusing) 4 lines of cmake? (I don't have a strong opinion about this; if you do invoke this via cmake I don't have a problem with keeping it in.)

In the past I just added it to my cmake invocation and since I used the same build area for a while it was just always being built. I asked in the original review why we couldn't just unconditionally generate it, but not include it so it was just always there as a reference. Not sure of the compile time cost of generating it.