This is an archive of the discontinued LLVM Phabricator instance.

[X86][NFC] Move MemoryFoldTable2Addr MemoryFoldTable0~4 into X86InstrFoldTables.def
ClosedPublic

Authored by yubing on Jan 18 2023, 10:57 PM.

Diff Detail

Event Timeline

yubing created this revision.Jan 18 2023, 10:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2023, 10:57 PM
yubing requested review of this revision.Jan 18 2023, 10:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptJan 18 2023, 10:57 PM
skan added inline comments.Jan 19 2023, 12:38 AM
llvm/lib/Target/X86/X86InstrFoldTables.def
1 ↗(On Diff #490379)

We should not define a C++ array in a def file.
Try sth like

#ifndef X86_MEM_FOLD_2ADDR
#define X86_MEM_FOLD_2ADDR(REG,MEM,FLAG)
X86_MEM_FOLD_2ADDR(ADD16ri8_DB,ADD16mi8,TB_NO_REVERSE)
X86_MEM_FOLD_2ADDR(ADD16ri_DB,ADD16mi,TB_NO_REVERSE)
#endif
#undef X86_MEM_FOLD_2ADDR

And in X86InstrFoldTables.cpp

static const X86MemoryFoldTableEntry MemoryFoldTable2Addr[] = {
#define X86_MEM_FOLD_2ADDR (REG,MEM,FLAG) {X86::REG, X86::MEM, FLAG},
}
craig.topper added inline comments.Jan 19 2023, 12:42 AM
llvm/lib/Target/X86/X86InstrFoldTables.def
1 ↗(On Diff #490379)

Could make it a .inc file instead of .def?

yubing added inline comments.Jan 19 2023, 12:51 AM
llvm/lib/Target/X86/X86InstrFoldTables.def
1 ↗(On Diff #490379)

but currently we still have some small difference between auto-gen table and the table in this file.
we will finally replace it with inc in the future patch

craig.topper added inline comments.Jan 19 2023, 12:55 AM
llvm/lib/Target/X86/X86InstrFoldTables.def
1 ↗(On Diff #490379)

.inc doesn't mean it has to come from tablegen. In llvm, .def is full of macros. You can name a file .inc in the the source directories.

yubing added inline comments.Jan 19 2023, 12:56 AM
llvm/lib/Target/X86/X86InstrFoldTables.def
1 ↗(On Diff #490379)

let's rename it into a .h since we still need to compare the manual table and generated table

1 ↗(On Diff #490379)

sounds good!

craig.topper added inline comments.Jan 19 2023, 12:59 AM
llvm/lib/Target/X86/X86InstrFoldTables.def
1 ↗(On Diff #490379)

A .h needs to have include guards. It can't just be the arrays.

yubing updated this revision to Diff 490409.Jan 19 2023, 1:21 AM

rename def as inc

pengfei added inline comments.Jan 19 2023, 2:32 AM
llvm/lib/Target/X86/X86MemFoldTables.inc
3

Can we format this file, or at least add two spaces for it?

skan added inline comments.Jan 19 2023, 2:36 AM
llvm/lib/Target/X86/X86MemFoldTables.inc
3

Can we format this file, or at least add two spaces for it?

You mean one space after the first and the second comma?

pengfei added inline comments.Jan 19 2023, 3:30 AM
llvm/lib/Target/X86/X86MemFoldTables.inc
3

No, just

static const X86MemoryFoldTableEntry MemoryFoldTable2Addr[] = {
  {X86::ADD16ri8_DB,X86::ADD16mi8,TB_NO_REVERSE},
RKSimon added inline comments.
llvm/lib/Target/X86/X86MemFoldTables.inc
3

column alignment would be even better

skan added inline comments.Jan 19 2023, 5:25 PM
llvm/lib/Target/X86/X86MemFoldTables.inc
3

Column aligment for huge array may bring issue for future development. For example, if we add a instruction with a long name, e.g X86::AAA_BBB_CCC_DDD_123_mi, then we have to format all the array to achive column aligment again.

yubing added inline comments.Feb 1 2023, 12:31 AM
llvm/lib/Target/X86/X86MemFoldTables.inc
3

Column alignment seems more reasonable, auto-gen mechanism can use OS.PadToColumn(40); if there is a longer instruction name in the future, we can make padding larger.

craig.topper added inline comments.Feb 1 2023, 12:40 AM
llvm/lib/Target/X86/X86MemFoldTables.inc
3

PadToColumn is kind of expensive. It requires updating a column counter during every print operation. Or at least it was on X86GenDAGISel.inc years ago before I removed it.

Since you’re printing a table it’s probably better to just print each string with padding. That should be cheaper than tracking a column. left_justify in Format.h might work

craig.topper added inline comments.Feb 1 2023, 12:46 AM
llvm/lib/Target/X86/X86MemFoldTables.inc
3

Old patch where I removed use of PadToColumn https://reviews.llvm.org/D37966. Might give some inspiration

yubing updated this revision to Diff 493891.Feb 1 2023, 3:24 AM

#!/bin/bash
cat $1 | awk '{if(NF>1) {gsub(/\t | /, "");print} else {print $0}}' FS=, | awk '{if(NF>1) {print " "$1", "$2", "$3","} else {print $0}}' FS=,

yubing added inline comments.Feb 1 2023, 3:28 AM
llvm/lib/Target/X86/X86MemFoldTables.inc
1

any format suggestion for line1?

pengfei accepted this revision.Feb 1 2023, 3:49 AM

LGTM.

This revision is now accepted and ready to land.Feb 1 2023, 3:49 AM
skan added inline comments.Feb 1 2023, 4:02 AM
llvm/lib/Target/X86/X86MemFoldTables.inc
1

[] = {

yubing updated this revision to Diff 493915.Feb 1 2023, 5:25 AM

small fix

skan accepted this revision.Feb 1 2023, 5:32 AM

LGTM

This revision was landed with ongoing or failed builds.Feb 2 2023, 12:39 AM
This revision was automatically updated to reflect the committed changes.