Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
| llvm/lib/Target/X86/X86InstrFoldTables.def | ||
|---|---|---|
| 1 ↗ | (On Diff #490379) | We should not define a C++ array in a def file. #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},
} |
| llvm/lib/Target/X86/X86InstrFoldTables.def | ||
|---|---|---|
| 1 ↗ | (On Diff #490379) | Could make it a .inc file instead of .def? |
| 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. |
| 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. |
| llvm/lib/Target/X86/X86InstrFoldTables.def | ||
|---|---|---|
| 1 ↗ | (On Diff #490379) | A .h needs to have include guards. It can't just be the arrays. |
| llvm/lib/Target/X86/X86MemFoldTables.inc | ||
|---|---|---|
| 3 | Can we format this file, or at least add two spaces for it? | |
| llvm/lib/Target/X86/X86MemFoldTables.inc | ||
|---|---|---|
| 3 |
You mean one space after the first and the second comma? | |
| llvm/lib/Target/X86/X86MemFoldTables.inc | ||
|---|---|---|
| 3 | No, just static const X86MemoryFoldTableEntry MemoryFoldTable2Addr[] = {
{X86::ADD16ri8_DB,X86::ADD16mi8,TB_NO_REVERSE}, | |
| llvm/lib/Target/X86/X86MemFoldTables.inc | ||
|---|---|---|
| 3 | column alignment would be even better | |
| 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. | |
| 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. | |
| 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 | |
| llvm/lib/Target/X86/X86MemFoldTables.inc | ||
|---|---|---|
| 3 | Old patch where I removed use of PadToColumn https://reviews.llvm.org/D37966. Might give some inspiration | |
#!/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=,
| llvm/lib/Target/X86/X86MemFoldTables.inc | ||
|---|---|---|
| 2 | any format suggestion for line1? | |
| llvm/lib/Target/X86/X86MemFoldTables.inc | ||
|---|---|---|
| 2 | [] = { | |
any format suggestion for line1?