[RFC][X86][MemFold] Upgrade the mechanism of auto-generated Memory Folding Table

[yubing]

Summary

1. Align ManualMapSet with X86MemoryFoldTableEntry instead of using UnfoldStrategy
2. ManualMapSet able to update the existing record in auto-generated MemFold table

[yubing]

llvm/lib/Target/X86/X86MemFoldTables.inc

4009 ↗

(On Diff #495799)

thanks for explanation. i also check the code in llvm/lib/CodeGen/MachineLICM.cpp

// First check whether we should hoist this instruction.
if (!IsLoopInvariantInst(*MI) || !IsProfitableToHoist(*MI)) {
  // If not, try unfolding a hoistable load.
  MI = ExtractHoistableLoad(MI);
  if (!MI) return false;
}

since VMOVAPDZ128rmk((the address isloop invariant but the mask is not)) is not IsLoopInvariantInst, we can extract a wholeload from it if it is Without TB_NO_REVERSE. so what you said about VMOVAPDZ128rmk and VMOVAPDZ128rm makes sense to me.

[yubing]

make X86::VMOVAPDZ128rrk, X86::VMOVAPDZ128rmk TB_NO_REVERSE

[yubing]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

422

Is there better way to check no-kz version's isMoveReg?

[yubing]

remove {"MMX_MOVQ64rr", "MMX_MOVQ64mr"}

[yubing]

disable {"MMX_MOVQ64rr", "MMX_MOVQ64rm"} as well

[yubing]

add comments for removing {"MMX_MOVQ64rr", "MMX_MOVQ64rm"}

[yubing]

the patch can let internal testcase pass. no regression was found in benchmark.

[yubing]

small rebase

[yubing]

ping? @skan

[skan]

LGTM in general

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

21

What's header file Error.h used for?

89

Drop the comment?

109–110

Is this operator never used?

133

Why do we need to compare isPseudo?

425

Do we need to assert the result of getDef is not NULL?

432

Why do we need Result.IsStore here?

[skan]

llvm/utils/TableGen/X86FoldTablesEmitterManualMapSet.inc

78

unfoldingtable -> unfolding table

81

ditto

[yubing]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

432

load such as VMOVAPDZ128rm will never be unfolded.
if VMOVAPDZ128rm 's memoperand is invariant, then VMOVAPDZ128rm will be hoisted directly and won't be unfolded into rm+rr
if VMOVAPDZ128rm 's memoperand is not invariant, we stop doing unfolding since compiler find it is a simple load.

// First check whether we should hoist this instruction.
if (!IsLoopInvariantInst(*MI) || !IsProfitableToHoist(*MI)) {
  // If not, try unfolding a hoistable load.
  MI = ExtractHoistableLoad(MI);
  if (!MI) return false;
}

MachineInstr *MachineLICMBase::ExtractHoistableLoad(MachineInstr *MI) {
  // Don't unfold simple loads.
  if (MI->canFoldAsLoad())
    return nullptr;

@craig.topper , does line429~439 make sense to you as well?

[yubing]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

133

I copy the code from "bool operator<(const X86FoldTableEntry &RHS) const {",
@craig.topper , i saw the author of "bool operator<(const X86FoldTableEntry &RHS) const {" is you. do you still remember why we need to compare isPseudo here?

[skan]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

133

I think it's a NFC change from me. If we remove this comparison, will any test fail?

[craig.topper]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

133

It's the sort order for the enum in X86GenInstrInfo.inc. Instructions with isPseudo set are ordered before other instructions.

[yubing]

does unfolding only happen in LICM?

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

432

@craig.topper i think my analysis of VMOVAPDZ128rm is only for LICM. but for other passes in codegen, is it possible for VMOVAPDZ128rm to be unfolded?

[craig.topper]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

432

The other 2 places I know that unfold are TwoAddressInstructionPass and SelectionDAG->MachineIR

TwoAddressInstuctionPass should only happen for instructions with tied source and dest. That doesn't apply to moveReg.

I think SelectionDAG->MachineIR case happens if we need to duplicate an instruction that has an EFLAGS def. If the EFLAGS are used by two instructions and EFLAGS is clobbered in between them. We need to duplicate the flag producing instruction to satisfy the second user. But we can't duplicate the folded load so we have to unfold. That doesn't apply to moveReg.

[yubing]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

432

i saw two kinds of unfoldMemoryOperand, one of which is for MIR and another is for DAG. Our folding table can't affect unfoldMemoryOperand for DAG, right?
and unfoldMemoryOperand for MIR only happens in LICM, TwoAddress, X86CMOVConversion, X86KCFI, and X86SpeculativeLoadHardening.
i checked VMOVAPDZ128rm won't be handled in those Pasess. so we don't worry if we need to set TB_NO_REVERSE for VMOVAPDZ128rm.

[yubing]

address skan's comments

[craig.topper]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

432

I think the DAG unfoldMemoryOperand uses the same table.

[craig.topper]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

432

The DAG unfold occurs after the isel portion of SelectionDAG. So it's MachineOpcodes

[yubing]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

432

oh, you're right. unfoldMemoryOperand is trying to find the MIR according to DAGNode's Machinecode, and then lookupUnfoldTable

bool
X86InstrInfo::unfoldMemoryOperand(SelectionDAG &DAG, SDNode *N,
                                  SmallVectorImpl<SDNode*> &NewNodes) const {
  if (!N->isMachineOpcode())
    return false;

  const X86MemoryFoldTableEntry *I = lookupUnfoldTable(N->getMachineOpcode());

@craig.topper , but where can i find the code of DAG unfold which occurs after the isel portion of SelectionDAG?
i saw unfoldMemoryOperand(for DAG) in llvm/lib/CodeGen/SelectionDAG/ScheduleDAGRRList.cpp

[craig.topper]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

432

ScheduleDAGRRList.cpp is the code. The DAG needs to be scheduled into linear basic block for MachineIR.

[yubing]

llvm/utils/TableGen/X86FoldTablesEmitterManualMapSet.inc

31

for myself, why " { "TAILJMPr", "TAILJMPm", TB_FOLDED_LOAD }," listed here.

[steven_wu]

llvm/test/TableGen/x86-auto-memfold.td

3

From the manpage of cmp, it should be like:

cmp [OPTIONS] file1 file2

There are some implementation that cannot take --ignore-initial after the files. Can you move it to the front?

[smeenai]

llvm/test/TableGen/x86-auto-memfold.td

3

This was breaking our tests on macOS as well, so I pushed rG7e271c2a8552 to fix.

[vitalybuka]

Breaks this bot https://lab.llvm.org/buildbot/#/builders/5/builds/32220/steps/16/logs/stdio

[yubing]

In D142084#4201278, @vitalybuka wrote:

Breaks this bot https://lab.llvm.org/buildbot/#/builders/5/builds/32220/steps/16/logs/stdio

@vitalybuka , do you know how to reproduce the issue by buildbot_fast.sh?
i tried https://github.com/google/sanitizers/wiki/SanitizerBotReproduceBuild#try-local-changes
but it set HEAD wrongly at

commit 8dfdcc7b7bf66834a761bd8de445840ef68e4d1a
Author: Nikolas Klauser <[e-mail]>
Date:   Thu Nov 17 21:34:29 2022 +0100

    [libc++] Fix memory leaks when throwing inside std::vector constructors

    Fixes #58392

    Reviewed By: ldionne, #libc

    Spies: alexfh, hans, joanahalili, dblaikie, libcxx-commits

    Differential Revision: https://reviews.llvm.org/D138601

[yubing]

@hctim do you know how to reproduce the ubsan issue elegantly. after i spend some time modify buildbot_functions.sh, i still can't reproduce it locally. https://lab.llvm.org/buildbot/#/builders/5/builds/32220/steps/16/logs/stdio

[craig.topper]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

386

Is it possible for S & TB_ALIGN_MASK to be 0? That would cause 1<< -1 which would match the ubsan error

[yubing]

address ubsan issue and ignore-initial issue

[craig.topper]

llvm/utils/TableGen/X86FoldTablesEmitter.cpp

387

Use a variable instead of repeating the same code twice?

[yubing]

address Craig's comments

[fdeazeve]

This commit may be breaking this bot:
https://green.lab.llvm.org/green/view/LLDB/job/lldb-cmake/52590/console

llvm-project/llvm/utils/TableGen/X86FoldTablesEmitterManualMapSet.inc:4:48: error: use of undeclared identifier 'TB_NO_REVERSE'
    { "ADD16ri_DB",         "ADD16mi",         TB_NO_REVERSE  },

[fdeazeve]

Also note that the pre-merge checks failed to clang format these files:

llvm/include/llvm/Support/X86FoldTablesUtils.h
llvm/lib/Target/X86/X86InstrFoldTables.h
llvm/utils/TableGen/X86FoldTablesEmitter.cpp
llvm/utils/TableGen/X86FoldTablesEmitterManualMapSet.inc

[fdeazeve]

llvm/include/llvm/Support/X86FoldTablesUtils.h

12

The anon namespace is causing the bot failure, was a there a particular reason for this change? If not, I'll submit a patch

[skan]

llvm/include/llvm/Support/X86FoldTablesUtils.h

12

I think it's a typo by the author of this patch.

[abhina.sreeskantharajan]

Hi, this is also causing a failure on the clang-ppc64-aix bot https://lab.llvm.org/buildbot/#/builders/214/builds/6519

+ : 'RUN: at line 1'
+ /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/bin/llvm-tblgen -gen-x86-fold-tables -asmwriternum=1 /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../lib/Target/X86/X86.td -I /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../include -I /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../lib/Target/X86/ -I /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../include/ -I /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../lib/Target/ --write-if-changed -o /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/test/TableGen/Output/x86-auto-memfold.td.tmp1
+ : 'RUN: at line 2'
+ cmp --ignore-initial=0:568 /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../lib/Target/X86/X86MemFoldTables.inc /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/test/TableGen/Output/x86-auto-memfold.td.tmp1
cmp: illegal option -- -
usage: cmp [-l | -s] file1 file2

[yubing]

llvm/include/llvm/Support/X86FoldTablesUtils.h

12

thanks for fixing it.

[yubing]

In D142084#4206765, @abhina.sreeskantharajan wrote:

Hi, this is also causing a failure on the clang-ppc64-aix bot https://lab.llvm.org/buildbot/#/builders/214/builds/6519

+ : 'RUN: at line 1'
+ /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/bin/llvm-tblgen -gen-x86-fold-tables -asmwriternum=1 /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../lib/Target/X86/X86.td -I /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../include -I /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../lib/Target/X86/ -I /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../include/ -I /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../lib/Target/ --write-if-changed -o /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/test/TableGen/Output/x86-auto-memfold.td.tmp1
+ : 'RUN: at line 2'
+ cmp --ignore-initial=0:568 /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/llvm-project/llvm/test/TableGen/../../lib/Target/X86/X86MemFoldTables.inc /scratch/powerllvm/powerllvm_env/aix-ppc64/clang-ppc64-aix/build/test/TableGen/Output/x86-auto-memfold.td.tmp1
cmp: illegal option -- -
usage: cmp [-l | -s] file1 file2

@abhina.sreeskantharajan
seems "--ignore-initial=0:568" doesn't support ppc64? will https://reviews.llvm.org/D146498 works for you?

[dblaikie]

llvm/test/TableGen/x86-auto-memfold.td

1

Could you update/fix this test to not depend on the real .td file - all the other tblgen tests include a small snippet/test case/example instead? (though the test was renamed in 35aeb321c005ed77ef5e5fb4f8dea69a81c81253)

[skan]

llvm/test/TableGen/x86-auto-memfold.td

1

@dblaikie Thank you for the suggestion! The test itself aims to expose the vulnerable rules in X86FoldTablesEmitter.cpp. Due to the complexity of X86ISA, even the folding rules are already complicated, it is hard to predict whether exceptions will occur when new ISAs are introduced. So unfortunately, we need the real .td file as input. I illustrated the usage of the test file in D147527