Skip to content

Commit

Permalink
Revert r313343 "[X86] PR32755 : Improvement in CodeGen instruction se…
Browse files Browse the repository at this point in the history 
…lection for LEAs."

This caused PR34629: asserts firing when building Chromium. It also broke some
buildbots building test-suite as reported on the commit thread.

> Summary:
>    1/  Operand folding during complex pattern matching for LEAs has been
>        extended, such that it promotes Scale to accommodate similar operand
>        appearing in the DAG.
>        e.g.
>           T1 = A + B
>           T2 = T1 + 10
>           T3 = T2 + A
>        For above DAG rooted at T3, X86AddressMode will no look like
>           Base = B , Index = A , Scale = 2 , Disp = 10
>
>    2/  During OptimizeLEAPass down the pipeline factorization is now performed over LEAs
>        so that if there is an opportunity then complex LEAs (having 3 operands)
>        could be factored out.
>        e.g.
>           leal 1(%rax,%rcx,1), %rdx
>           leal 1(%rax,%rcx,2), %rcx
>        will be factored as following
>           leal 1(%rax,%rcx,1), %rdx
>           leal (%rdx,%rcx)   , %edx
>
>    3/ Aggressive operand folding for AM based selection for LEAs is sensitive to loops,
>       thus avoiding creation of any complex LEAs within a loop.
>
> Reviewers: lsaba, RKSimon, craig.topper, qcolombet
>
> Reviewed By: lsaba
>
> Subscribers: spatel, igorb, llvm-commits
>
> Differential Revision: https://reviews.llvm.org/D35014

git-svn-id: https://llvm.org/svn/llvm-project/llvm/trunk@313376 91177308-0d34-0410-b5e6-96231b3b80d8
  • Loading branch information
@zmodem
zmodem committed Sep 15, 2017
1 parent ab804ca commit 126ab47
Show file tree
Hide file tree
Showing 18 changed files with 176 additions and 612 deletions.
3 changes: 1 addition & 2 deletions include/llvm/CodeGen/MachineInstr.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -1289,13 +1289,12 @@ class MachineInstr
/// Add all implicit def and use operands to this instruction.
void addImplicitDefUseOperands(MachineFunction &MF);

private:
/// If this instruction is embedded into a MachineFunction, return the
/// MachineRegisterInfo object for the current function, otherwise
/// return null.
MachineRegisterInfo *getRegInfo();

private:

/// Unlink all of the register operands in this instruction from their
/// respective use lists. This requires that the operands already be on their
/// use lists.
Expand Down
3 changes: 0 additions & 3 deletions include/llvm/CodeGen/SelectionDAG.h
View file
Original file line number Diff line number Diff line change
Expand Up @@ -300,9 +300,6 @@ class SelectionDAG {
/// type legalization.
bool NewNodesMustHaveLegalTypes = false;

/// Set to true for DAG of BasicBlock contained inside a loop.
bool IsDAGPartOfLoop = false;

private:
/// DAGUpdateListener is a friend so it can manipulate the listener stack.
friend struct DAGUpdateListener;
Expand Down
11 changes: 0 additions & 11 deletions lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@
#include "llvm/Analysis/AliasAnalysis.h"
#include "llvm/Analysis/BranchProbabilityInfo.h"
#include "llvm/Analysis/CFG.h"
#include "llvm/Analysis/LoopInfo.h"
#include "llvm/Analysis/OptimizationDiagnosticInfo.h"
#include "llvm/Analysis/TargetLibraryInfo.h"
#include "llvm/CodeGen/FastISel.h"
Expand Down Expand Up @@ -326,8 +325,6 @@ void SelectionDAGISel::getAnalysisUsage(AnalysisUsage &AU) const {
if (OptLevel != CodeGenOpt::None)
AU.addRequired<AAResultsWrapperPass>();
AU.addRequired<GCModuleInfo>();
if (OptLevel != CodeGenOpt::None)
AU.addRequired<LoopInfoWrapperPass>();
AU.addRequired<StackProtector>();
AU.addPreserved<StackProtector>();
AU.addPreserved<GCModuleInfo>();
Expand Down Expand Up @@ -1422,7 +1419,6 @@ void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {

// Iterate over all basic blocks in the function.
for (const BasicBlock *LLVMBB : RPOT) {
CurDAG->IsDAGPartOfLoop = false;
if (OptLevel != CodeGenOpt::None) {
bool AllPredsVisited = true;
for (const_pred_iterator PI = pred_begin(LLVMBB), PE = pred_end(LLVMBB);
Expand Down Expand Up @@ -1600,13 +1596,6 @@ void SelectionDAGISel::SelectAllBasicBlocks(const Function &Fn) {
FunctionBasedInstrumentation);
}

if (OptLevel != CodeGenOpt::None) {
auto &LIWP = getAnalysis<LoopInfoWrapperPass>();
LoopInfo &LI = LIWP.getLoopInfo();
if (LI.getLoopFor(LLVMBB))
CurDAG->IsDAGPartOfLoop = true;
}

if (Begin != BI)
++NumDAGBlocks;
else
Expand Down
85 changes: 6 additions & 79 deletions lib/Target/X86/X86ISelDAGToDAG.cpp
View file
Original file line number Diff line number Diff line change
Expand Up @@ -88,11 +88,6 @@ namespace {
IndexReg.getNode() != nullptr || Base_Reg.getNode() != nullptr;
}

bool hasComplexAddressingMode() const {
return Disp && IndexReg.getNode() != nullptr &&
Base_Reg.getNode() != nullptr;
}

/// Return true if this addressing mode is already RIP-relative.
bool isRIPRelative() const {
if (BaseType != RegBase) return false;
Expand All @@ -102,10 +97,6 @@ namespace {
return false;
}

bool isLegalScale() {
return (Scale == 1 || Scale == 2 || Scale == 4 || Scale == 8);
}

void setBaseReg(SDValue Reg) {
BaseType = RegBase;
Base_Reg = Reg;
Expand Down Expand Up @@ -171,13 +162,10 @@ namespace {
/// If true, selector should try to optimize for minimum code size.
bool OptForMinSize;

/// If true, selector should try to aggresively fold operands into AM.
bool OptForAggressingFolding;

public:
explicit X86DAGToDAGISel(X86TargetMachine &tm, CodeGenOpt::Level OptLevel)
: SelectionDAGISel(tm, OptLevel), OptForSize(false),
OptForMinSize(false), OptForAggressingFolding(false) {}
OptForMinSize(false) {}

StringRef getPassName() const override {
return "X86 DAG->DAG Instruction Selection";
Expand All @@ -196,12 +184,6 @@ namespace {

void PreprocessISelDAG() override;

void setAggressiveOperandFolding(bool val = false) {
OptForAggressingFolding = val;
}

bool getAggressiveOperandFolding() { return OptForAggressingFolding; }

// Include the pieces autogenerated from the target description.
#include "X86GenDAGISel.inc"

Expand All @@ -215,7 +197,6 @@ namespace {
bool matchAdd(SDValue N, X86ISelAddressMode &AM, unsigned Depth);
bool matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
unsigned Depth);
bool matchAddressLEA(SDValue N, X86ISelAddressMode &AM);
bool matchAddressBase(SDValue N, X86ISelAddressMode &AM);
bool selectAddr(SDNode *Parent, SDValue N, SDValue &Base,
SDValue &Scale, SDValue &Index, SDValue &Disp,
Expand Down Expand Up @@ -444,20 +425,6 @@ namespace {

bool matchBEXTRFromAnd(SDNode *Node);
};

class X86AggressiveOperandFolding {
public:
explicit X86AggressiveOperandFolding(X86DAGToDAGISel &ISel, bool val)
: Selector(&ISel) {
Selector->setAggressiveOperandFolding(val);
}
~X86AggressiveOperandFolding() {
Selector->setAggressiveOperandFolding(false);
}

private:
X86DAGToDAGISel *Selector;
};
}


Expand Down Expand Up @@ -1171,19 +1138,16 @@ static bool foldMaskAndShiftToScale(SelectionDAG &DAG, SDValue N,
AM.IndexReg = NewSRL;
return false;
}

bool X86DAGToDAGISel::matchAddressRecursively(SDValue N, X86ISelAddressMode &AM,
unsigned Depth) {
SDLoc dl(N);
DEBUG({
dbgs() << "MatchAddress: ";
AM.dump();
});

// Limit recursion. For aggressive operand folding recurse
// till depth 8 which is the maximum legal scale value.
unsigned MaxDepth = getAggressiveOperandFolding() ? 8 : 5;
if (Depth > MaxDepth)
// Limit recursion.
if (Depth > 5)
return matchAddressBase(N, AM);

// If this is already a %rip relative address, we can only merge immediates
Expand Down Expand Up @@ -1474,20 +1438,6 @@ bool X86DAGToDAGISel::matchAddressBase(SDValue N, X86ISelAddressMode &AM) {
return false;
}

if (OptLevel != CodeGenOpt::None && getAggressiveOperandFolding() &&
AM.BaseType == X86ISelAddressMode::RegBase) {
if (AM.Base_Reg == N) {
SDValue Base_Reg = AM.Base_Reg;
AM.Base_Reg = AM.IndexReg;
AM.IndexReg = Base_Reg;
AM.Scale++;
return false;
} else if (AM.IndexReg == N) {
AM.Scale++;
return false;
}
}

// Otherwise, we cannot select it.
return true;
}
Expand Down Expand Up @@ -1718,7 +1668,7 @@ bool X86DAGToDAGISel::selectLEA64_32Addr(SDValue N, SDValue &Base,
SDValue &Disp, SDValue &Segment) {
// Save the debug loc before calling selectLEAAddr, in case it invalidates N.
SDLoc DL(N);

if (!selectLEAAddr(N, Base, Scale, Index, Disp, Segment))
return false;

Expand Down Expand Up @@ -1753,29 +1703,6 @@ bool X86DAGToDAGISel::selectLEA64_32Addr(SDValue N, SDValue &Base,
return true;
}

bool X86DAGToDAGISel::matchAddressLEA(SDValue N, X86ISelAddressMode &AM) {
// Avoid enabling aggressive operand folding when node N is a part of loop.
X86AggressiveOperandFolding Enable(*this, !CurDAG->IsDAGPartOfLoop);

bool matchRes = matchAddress(N, AM);

// Check for legality of scale when recursion unwinds back to the top.
if (!matchRes) {
if (!AM.isLegalScale())
return true;

// Avoid creating costly complex LEAs having scale less than 2
// within loop.
if(CurDAG->IsDAGPartOfLoop && Subtarget->slow3OpsLEA() &&
AM.Scale <= 2 && AM.hasComplexAddressingMode() &&
(!AM.hasSymbolicDisplacement() && N.getOpcode() < ISD::BUILTIN_OP_END))
return true;
}

return matchRes;
}


/// Calls SelectAddr and determines if the maximal addressing
/// mode it matches can be cost effectively emitted as an LEA instruction.
bool X86DAGToDAGISel::selectLEAAddr(SDValue N,
Expand All @@ -1793,7 +1720,7 @@ bool X86DAGToDAGISel::selectLEAAddr(SDValue N,
SDValue Copy = AM.Segment;
SDValue T = CurDAG->getRegister(0, MVT::i32);
AM.Segment = T;
if (matchAddressLEA(N, AM))
if (matchAddress(N, AM))
return false;
assert (T == AM.Segment);
AM.Segment = Copy;
Expand Down
Loading

0 comments on commit 126ab47

Please sign in to comment.