Page MenuHomePhabricator
Differential D72225 Diff 236526 llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

Changeset View

Standalone View

View Options

llvm/lib/Target/X86/MCTargetDesc/X86AsmBackend.cpp

//===-- X86AsmBackend.cpp - X86 Assembler Backend -------------------------===// //===-- X86AsmBackend.cpp - X86 Assembler Backend -------------------------===//
// //
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions. // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
// See https://llvm.org/LICENSE.txt for license information. // See https://llvm.org/LICENSE.txt for license information.
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
// //
//===----------------------------------------------------------------------===// //===----------------------------------------------------------------------===//
#include "MCTargetDesc/X86BaseInfo.h" #include "MCTargetDesc/X86BaseInfo.h"
#include "MCTargetDesc/X86FixupKinds.h" #include "MCTargetDesc/X86FixupKinds.h"
#include "llvm/ADT/StringSwitch.h" #include "llvm/ADT/StringSwitch.h"
#include "llvm/BinaryFormat/ELF.h" #include "llvm/BinaryFormat/ELF.h"
#include "llvm/BinaryFormat/MachO.h" #include "llvm/BinaryFormat/MachO.h"
#include "llvm/MC/MCAsmBackend.h" #include "llvm/MC/MCAsmBackend.h"
#include "llvm/MC/MCAssembler.h" #include "llvm/MC/MCAssembler.h"
#include "llvm/MC/MCCodeEmitter.h"
#include "llvm/MC/MCContext.h" #include "llvm/MC/MCContext.h"
#include "llvm/MC/MCDwarf.h" #include "llvm/MC/MCDwarf.h"
#include "llvm/MC/MCELFObjectWriter.h" #include "llvm/MC/MCELFObjectWriter.h"
#include "llvm/MC/MCExpr.h" #include "llvm/MC/MCExpr.h"
#include "llvm/MC/MCFixupKindInfo.h" #include "llvm/MC/MCFixupKindInfo.h"
#include "llvm/MC/MCInst.h" #include "llvm/MC/MCInst.h"
#include "llvm/MC/MCInstrInfo.h" #include "llvm/MC/MCInstrInfo.h"
#include "llvm/MC/MCMachObjectWriter.h" #include "llvm/MC/MCMachObjectWriter.h"
#include "llvm/MC/MCObjectStreamer.h" #include "llvm/MC/MCObjectStreamer.h"
#include "llvm/MC/MCObjectWriter.h" #include "llvm/MC/MCObjectWriter.h"
#include "llvm/MC/MCRegisterInfo.h" #include "llvm/MC/MCRegisterInfo.h"
#include "llvm/MC/MCSectionMachO.h" #include "llvm/MC/MCSectionMachO.h"
#include "llvm/MC/MCSubtargetInfo.h" #include "llvm/MC/MCSubtargetInfo.h"
#include "llvm/MC/MCValue.h" #include "llvm/MC/MCValue.h"
#include "llvm/Support/CommandLine.h" #include "llvm/Support/CommandLine.h"
#include "llvm/Support/ErrorHandling.h" #include "llvm/Support/ErrorHandling.h"
#include "llvm/Support/raw_ostream.h"
#include "llvm/Support/TargetRegistry.h" #include "llvm/Support/TargetRegistry.h"
#include "llvm/Support/raw_ostream.h"
reamesUnsubmitted
Not Done
ReplyInline Actions

Remove the reordering here.

reames: Remove the reordering here.
using namespace llvm; using namespace llvm;
static unsigned getFixupKindSize(unsigned Kind) { static unsigned getFixupKindSize(unsigned Kind) {
switch (Kind) { switch (Kind) {
default: default:
llvm_unreachable("invalid fixup kind!"); llvm_unreachable("invalid fixup kind!");
case FK_NONE: case FK_NONE:
▲ Show 20 Lines • Show All 72 Lines • ▼ Show 20 Linespublic:
operator uint8_t() const { return AlignBranchKind; } operator uint8_t() const { return AlignBranchKind; }
void addKind(Flag Value) { AlignBranchKind |= Value; } void addKind(Flag Value) { AlignBranchKind |= Value; }
}; };
X86AlignBranchKind X86AlignBranchKindLoc; X86AlignBranchKind X86AlignBranchKindLoc;
cl::opt<unsigned> X86AlignBranchBoundary( cl::opt<unsigned> X86AlignBranchBoundary(
"x86-align-branch-boundary", cl::init(0), "x86-align-branch-boundary", cl::init(0),
cl::desc( cl::desc("Control how the assembler should align branches with NOP or "
"Control how the assembler should align branches with NOP. If the " "segment override prefix. If the boundary's size is not 0, it "
"boundary's size is not 0, it should be a power of 2 and no less " "should be a power of 2 and no less than 32. Branches will be "
"than 32. Branches will be aligned within the boundary of specified " "aligned within the boundary of specified size. "
"size. -x86-align-branch-boundary=0 doesn't align branches.")); "-x86-align-branch-boundary=0 doesn't align branches."));
reamesUnsubmitted
Done
ReplyInline Actions

a) segment prefixes aren't the only ones used are they?
b) the wording changes can be pulled into their own review (or simply committed)

reames: a) segment prefixes aren't the only ones used are they? b) the wording changes can be pulled…
skanAuthorUnsubmitted
Done
ReplyInline Actions

If there is no room to insert prefix, NOP will be emitted before the branch or fused pair.

skan: If there is no room to insert prefix, NOP will be emitted before the branch or fused pair.
cl::opt<X86AlignBranchKind, true, cl::parser<std::string>> X86AlignBranch( cl::opt<X86AlignBranchKind, true, cl::parser<std::string>> X86AlignBranch(
"x86-align-branch", "x86-align-branch",
cl::desc("Specify types of branches to align (plus separated list of " cl::desc("Specify types of branches to align (plus separated list of "
"types). The branches's type is combination of jcc, fused, " "types). The branches's type is combination of jcc, fused, "
"jmp, call, ret, indirect."), "jmp, call, ret, indirect."),
cl::value_desc("jcc(conditional jump), fused(fused conditional jump), " cl::value_desc("jcc(conditional jump), fused(fused conditional jump), "
"jmp(unconditional jump); call(call); ret(ret), " "jmp(unconditional jump); call(call); ret(ret), "
"indirect(indirect jump)."), "indirect(indirect jump)."),
cl::location(X86AlignBranchKindLoc)); cl::location(X86AlignBranchKindLoc));
cl::opt<unsigned> X86AlignBranchPrefixSize(
"x86-align-branch-prefix-size", cl::init(0), cl::Hidden,
cl::desc("Specify the maximum number of prefixes on an instruction to "
"align branches. The number should be between 0 and 5."));
class X86ELFObjectWriter : public MCELFObjectTargetWriter { class X86ELFObjectWriter : public MCELFObjectTargetWriter {
public: public:
X86ELFObjectWriter(bool is64Bit, uint8_t OSABI, uint16_t EMachine, X86ELFObjectWriter(bool is64Bit, uint8_t OSABI, uint16_t EMachine,
bool HasRelocationAddend, bool foobar) bool HasRelocationAddend, bool foobar)
: MCELFObjectTargetWriter(is64Bit, OSABI, EMachine, HasRelocationAddend) {} : MCELFObjectTargetWriter(is64Bit, OSABI, EMachine, HasRelocationAddend) {}
}; };
class X86AsmBackend : public MCAsmBackend { class X86AsmBackend : public MCAsmBackend {
const MCSubtargetInfo &STI; const MCSubtargetInfo &STI;
std::unique_ptr<const MCInstrInfo> MCII; std::unique_ptr<const MCInstrInfo> MCII;
X86AlignBranchKind AlignBranchType; X86AlignBranchKind AlignBranchType;
Align AlignBoundary; Align AlignBoundary;
uint8_t AlignMaxPrefixSize;
bool isMacroFused(const MCInst &Cmp, const MCInst &Jcc) const; bool isMacroFused(const MCInst &Cmp, const MCInst &Jcc) const;
bool needAlign(MCObjectStreamer &OS) const;
bool needAlignInst(const MCInst &Inst) const; bool needAlignInst(const MCInst &Inst) const;
MCBoundaryAlignFragment *
getOrCreateBoundaryAlignFragment(MCObjectStreamer &OS) const; bool shouldAddPrefix(const MCInst &Inst) const;
uint8_t choosePrefix(const MCInst &Inst) const;
MCInst PrevInst; MCInst PrevInst;
const MCFragment *LastFragmentToBeAligned = nullptr;
public: public:
X86AsmBackend(const Target &T, const MCSubtargetInfo &STI) X86AsmBackend(const Target &T, const MCSubtargetInfo &STI)
: MCAsmBackend(support::little), STI(STI), : MCAsmBackend(support::little), STI(STI),
MCII(T.createMCInstrInfo()) { MCII(T.createMCInstrInfo()) {
AlignBoundary = assumeAligned(X86AlignBranchBoundary); AlignBoundary = assumeAligned(X86AlignBranchBoundary);
AlignBranchType = X86AlignBranchKindLoc; AlignBranchType = X86AlignBranchKindLoc;
AlignMaxPrefixSize = std::min<uint8_t>(X86AlignBranchPrefixSize, 5);
MaskRayUnsubmitted
Not Done
ReplyInline Actions

AlignMaxPrefixSize = X86AlignBranchPrefixSize;

The error checking and normalization (to 5) should be done in an earlier place.

MaskRay: `AlignMaxPrefixSize = X86AlignBranchPrefixSize;` The error checking and normalization (to 5)…
skanAuthorUnsubmitted
Done
ReplyInline Actions

Do you any suggestions about a appropriate, earlier place?

skan: Do you any suggestions about a appropriate, earlier place?
} }
void alignBranchesBegin(MCObjectStreamer &OS, const MCInst &Inst) override; void alignBranchesBegin(MCObjectStreamer &OS, const MCInst &Inst) override;
void alignBranchesEnd(MCObjectStreamer &OS, const MCInst &Inst) override; void alignBranchesEnd(MCObjectStreamer &OS, const MCInst &Inst) override;
bool needAlign(MCObjectStreamer &OS) const;
unsigned getNumFixupKinds() const override { unsigned getNumFixupKinds() const override {
return X86::NumTargetFixupKinds; return X86::NumTargetFixupKinds;
} }
Optional<MCFixupKind> getFixupKind(StringRef Name) const override; Optional<MCFixupKind> getFixupKind(StringRef Name) const override;
const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override { const MCFixupKindInfo &getFixupKindInfo(MCFixupKind Kind) const override {
▲ Show 20 Lines • Show All 249 Lines • ▼ Show 20 Linesbool X86AsmBackend::needAlign(MCObjectStreamer &OS) const {
MCAssembler &Assembler = OS.getAssembler(); MCAssembler &Assembler = OS.getAssembler();
MCSection *Sec = OS.getCurrentSectionOnly(); MCSection *Sec = OS.getCurrentSectionOnly();
// To be Done: Currently don't deal with Bundle cases. // To be Done: Currently don't deal with Bundle cases.
if (Assembler.isBundlingEnabled() && Sec->isBundleLocked()) if (Assembler.isBundlingEnabled() && Sec->isBundleLocked())
return false; return false;
// Branches only need to be aligned in 32-bit or 64-bit mode. // Branches only need to be aligned in 32-bit or 64-bit mode.
if (!(STI.getFeatureBits()[X86::Mode64Bit] || if (!(STI.hasFeature(X86::Mode64Bit) || STI.hasFeature(X86::Mode32Bit)))
reamesUnsubmitted
Done
ReplyInline Actions

This looks to be a formatting change? If so, remove. You can commit this without separate review if desired.

reames: This looks to be a formatting change? If so, remove. You can commit this without separate…
STI.getFeatureBits()[X86::Mode32Bit]))
return false; return false;
return true; return true;
} }
/// Check if the instruction operand needs to be aligned. Padding is disabled /// Check if the instruction operand needs to be aligned. Padding is disabled
/// before intruction which may be rewritten by linker(e.g. TLSCALL). /// before intruction which may be rewritten by linker(e.g. TLSCALL).
bool X86AsmBackend::needAlignInst(const MCInst &Inst) const { bool X86AsmBackend::needAlignInst(const MCInst &Inst) const {
Show All 9 Lines return (InstDesc.isConditionalBranch() &&
(InstDesc.isCall() && (InstDesc.isCall() &&
(AlignBranchType & X86AlignBranchKind::AlignBranchCall)) || (AlignBranchType & X86AlignBranchKind::AlignBranchCall)) ||
(InstDesc.isReturn() && (InstDesc.isReturn() &&
(AlignBranchType & X86AlignBranchKind::AlignBranchRet)) || (AlignBranchType & X86AlignBranchKind::AlignBranchRet)) ||
(InstDesc.isIndirectBranch() && (InstDesc.isIndirectBranch() &&
(AlignBranchType & X86AlignBranchKind::AlignBranchIndirect)); (AlignBranchType & X86AlignBranchKind::AlignBranchIndirect));
} }
static bool canReuseBoundaryAlignFragment(const MCBoundaryAlignFragment &F) { /// Check if prefix can be added before instruction \p Inst.
// If a MCBoundaryAlignFragment has not been used to emit NOP,we can reuse it. bool X86AsmBackend::shouldAddPrefix(const MCInst &Inst) const {
return !F.canEmitNops(); // No prefix can be added if AlignMaxPrefixSize is 0.
if (AlignMaxPrefixSize == 0)
return false;
// The longer the instruction, the easier it is to cross boundary, prefixes
// should not be inserted before any branch affected by JCC Erratum even if it
// is asked to be aligned.
const MCInstrDesc &InstDesc = MCII->get(Inst.getOpcode());
reamesUnsubmitted
Done
ReplyInline Actions

This should probably be:
if (needAlign(Inst))

return false;
reames: This should probably be: if (needAlign(Inst)) return false;
if (InstDesc.isBranch() || InstDesc.isCall() || InstDesc.isReturn())
return false;
// Linker may rewrite the instruction with variant symbol operand.
return !hasVariantSymbol(Inst);
}
/// Choose which prefix should be inserted before the instruction. The choice of
/// prefixes are:
/// a. Use the existing segment prefix if there is one.
/// b. Use CS segment prefix in 64-bit mode.
/// c. In 32-bit mode, use SS segment prefix with ESP/EBP base register and use
/// DS segment prefix without ESP/EBP base register.
uint8_t X86AsmBackend::choosePrefix(const MCInst &Inst) const {
for (const auto &Operand : Inst) {
if (Operand.isReg())
switch (Operand.getReg()) {
default:
craig.topperUnsubmitted
Done
ReplyInline Actions

I think this loop picks up instructions that don't use segment registers for just memory. And probably picks the wrong prefix for this instruction

mov %fs:0x1, %ss

The loop will find the %ss destination register first. But the %fs is the correct segment to use.

craig.topper: I think this loop picks up instructions that don't use segment registers for just memory. And…
break;
case X86::CS:
return 0x2e;
reamesUnsubmitted
Not Done
ReplyInline Actions

Please define an enum which gives a symbolic name for these values (if there isn't already one). (i.e. what the heck are these integer constant values?)

reames: Please define an enum which gives a symbolic name for these values (if there isn't already one).
skanAuthorUnsubmitted
Done
ReplyInline Actions

It is the exact value to be emitted when it has corresponding segment prefix. See X86MCCodeEmitter::emitSegmentOverridePrefix

skan: It is the exact value to be emitted when it has corresponding segment prefix. See…
case X86::SS:
return 0x36;
case X86::DS:
return 0x3e;
case X86::ES:
return 0x26;
case X86::FS:
return 0x64;
case X86::GS:
return 0x65;
}
} }
if (STI.hasFeature(X86::Mode64Bit))
return 0x2e;
MCBoundaryAlignFragment * unsigned Opcode = Inst.getOpcode();
X86AsmBackend::getOrCreateBoundaryAlignFragment(MCObjectStreamer &OS) const { const MCInstrDesc &Desc = MCII->get(Opcode);
auto *F = dyn_cast_or_null<MCBoundaryAlignFragment>(OS.getCurrentFragment()); uint64_t TSFlags = Desc.TSFlags;
if (!F || !canReuseBoundaryAlignFragment(*F)) { int MemoryOperand = X86II::getMemoryOperandNo(TSFlags);
F = new MCBoundaryAlignFragment(AlignBoundary); if (MemoryOperand >= 0) {
OS.insert(F); unsigned CurOp = X86II::getOperandBias(Desc);
unsigned BaseRegNum = MemoryOperand + CurOp + X86::AddrBaseReg;
unsigned BaseReg = Inst.getOperand(BaseRegNum).getReg();
if (BaseReg == X86::ESP || BaseReg == X86::EBP)
return 0x36;
} }
return F; return 0x3e;
} }
/// Insert MCBoundaryAlignFragment before instructions to align branches. /// Insert MCBoundaryAlignFragment before instructions to align branches.
void X86AsmBackend::alignBranchesBegin(MCObjectStreamer &OS, void X86AsmBackend::alignBranchesBegin(MCObjectStreamer &OS,
const MCInst &Inst) { const MCInst &Inst) {
if (!needAlign(OS)) if (!needAlign(OS))
return; return;
// Summary of inserting scheme:
// If the previous instruction is the first instruction in a fusible pair
// - If macro fusion actually happens, emit NOP before the first instrucion
// in the fused pair.
// - If the macro fusion doesn't happen indeed, emit prefix before the
// previous instruction and emit NOP before the instruction to be aligned.
//
// If the instruction needs to be aligned, emit NOP before the instruction.
//
// If the instruction is the first instruction in a fusible pair, put a
// a placeholder here.
//
// Otherwise emit prefix before the instruction if possible.
MCFragment *CF = OS.getCurrentFragment(); MCFragment *CF = OS.getCurrentFragment();
// Prefix or NOP shouldn't be inserted after hardcode, e.g.
//
// \code
// .byte 0x2e
// jmp .Label0
// \endcode
//
// since there is no clear instruction boundary.
if (isa_and_nonnull<MCDataFragment>(CF) && !CF->hasInstructions()) {
reamesUnsubmitted
Not Done
ReplyInline Actions

Please remove this. It should be covered by the compiler support patch which already landed for compiled code, and he assembler syntax is separate.

reames: Please remove this. It should be covered by the compiler support patch which already landed…
skanAuthorUnsubmitted
Done
ReplyInline Actions

Remove this will cause test align-branch-32-2a.s to fail. This check is simply and direct, I think we can keep this currently.

skan: Remove this will cause test align-branch-32-2a.s to fail. This check is simply and direct, I…
PrevInst = Inst;
return;
}
// The number of prefix is limted by AlignMaxPrefixSize for some peformance
// reasons, so we need to compute how many prefixes can be added.
auto GetRemainingPrefixSize = [&](const MCInst &Inst) {
SmallString<256> Code;
raw_svector_ostream VecOS(Code);
reamesUnsubmitted
Done
ReplyInline Actions

Please assert that the total number of prefixes fits within a uint8_t. It does, but having that explicitly asserted/noted would be helpful.

reames: Please assert that the total number of prefixes fits within a uint8_t. It does, but having…
OS.getAssembler().getEmitter().emitPrefix(Inst, VecOS, STI);
uint8_t ExistingPrefixSize = static_cast<uint8_t>(Code.size());
return (AlignMaxPrefixSize > ExistingPrefixSize)
? (AlignMaxPrefixSize - ExistingPrefixSize)
: 0;
};
bool NeedAlignFused = AlignBranchType & X86AlignBranchKind::AlignBranchFused; bool NeedAlignFused = AlignBranchType & X86AlignBranchKind::AlignBranchFused;
if (NeedAlignFused && isMacroFused(PrevInst, Inst) && CF) { // Handle the condition when the previous the instruction is the first
// Macro fusion actually happens and there is no other fragment inserted // instruction in a fusible pair. We need to check the previous fragment
// after the previous instruction. NOP can be emitted in PF to align fused // is a BF since we may encounter the case:
// jcc.
if (auto *PF =
dyn_cast_or_null<MCBoundaryAlignFragment>(CF->getPrevNode())) {
const_cast<MCBoundaryAlignFragment *>(PF)->setEmitNops(true);
const_cast<MCBoundaryAlignFragment *>(PF)->setFused(true);
}
} else if (needAlignInst(Inst)) {
// Note: When there is at least one fragment, such as MCAlignFragment,
// inserted after the previous instruction, e.g.
// //
// \code // \code
// cmp %rax %rcx // cmp %rax %rcx
// .align 16 // .align 16
// je .Label0 // je .Label0
// \ endcode // \endcode
// //
// We will treat the JCC as a unfused branch although it may be fused // MCAlignFragment can grow and shrink, so it is not ensured to get a fixed
// with the CMP. // size after finite times of relaxation. NOP should not emitted before the
auto *F = getOrCreateBoundaryAlignFragment(OS); // CMP since it may cause MCAssembler::relaxBoundaryAlign not to converge.
craig.topperUnsubmitted
Done
ReplyInline Actions

limted->limited
peformance->performance

craig.topper: limted->limited peformance->performance
if (NeedAlignFused && isFirstMacroFusibleInst(PrevInst, *MCII) && CF &&
isa_and_nonnull<MCBoundaryAlignFragment>(CF->getPrevNode())) {
auto *PF = const_cast<MCBoundaryAlignFragment *>(
cast<MCBoundaryAlignFragment>(CF->getPrevNode()));
// Macro fusion actually happens, so emit NOP before the first instrucion in
// the fused pair.
if (isMacroFused(PrevInst, Inst)) {
PF->setAlignment(AlignBoundary);
PF->setEmitNops(true);
} else if (shouldAddPrefix(PrevInst)) {
// Macro fusion doesn't happen indeed, emit prefix before the previous
// instruction and emit NOP before the instruction to be aligned.
PF->setAlignment(AlignBoundary);
PF->setMaxBytesToEmit(GetRemainingPrefixSize(PrevInst));
PF->setValue(choosePrefix(PrevInst));
auto *F = OS.getOrCreateBoundaryAlignFragment();
F->setAlignment(AlignBoundary);
F->setEmitNops(true);
}
} else if (needAlignInst(Inst) && !isa_and_nonnull<MCAlignFragment>(CF)) {
// Handle the condition when the instruction to be aligned is unfused. When
craig.topperUnsubmitted
Done
ReplyInline Actions

Your emitPrefix function includes the 2 byte and 3 byte VEX prefixes, the 4 byte EVEX prefix, and the 3 byte XOP prefix. The bytes after the leading byte of those prefixes can be any byte value and does not indicate a segment value.

craig.topper: Your emitPrefix function includes the 2 byte and 3 byte VEX prefixes, the 4 byte EVEX prefix…
// there is a MCAlignFragment inserted just before the instruction to
// be aligned, e.g.
//
// \code
// .align 16
// je .Label0
// \endcode
//
// We will not emit NOP before the instruction since the align directive is
// used to align JCC rather than NOP.
//
LuoYuankeUnsubmitted
Done
ReplyInline Actions

I notice line 623 also check the MCAlignFragment. Is it better to check MCAlignFragment at the beginning, and return without insert any MCBoundaryAlignFragment if the previous fragment is MCAlignFragment? In the following code, we can assume the previous fragment is not MCAlignFragment.

LuoYuanke: I notice line 623 also check the MCAlignFragment. Is it better to check MCAlignFragment at the…
skanAuthorUnsubmitted
Done
ReplyInline Actions

We shouldn't do that. Only NOP should not be emitted after a MCAlignFragment, since it the MCAlignFragment is used to align the branch or the fused pair rather than NOP. However, prefix can be emitted after a MCAlignFragment, since it is the part of the instruction.

skan: We shouldn't do that. Only NOP should not be emitted after a `MCAlignFragment`, since it the…
// Emit NOP before the instruction to be aligned.
auto *F = OS.getOrCreateBoundaryAlignFragment();
MaskRayUnsubmitted
Done
ReplyInline Actions

You can remove PF->setAlignment(AlignBoundary); here.

MaskRay: You can remove `PF->setAlignment(AlignBoundary);` here.
skanAuthorUnsubmitted
Done
ReplyInline Actions

The MCBoundaryAlignFragment may not emit anything, so the constructor of MCBoundaryAlignFragment doesn't set the AlignBoundary with value provided by the user. The data member AlignBoundary will be not corretly set here if PF->setAlignment(AlignBoundary) is removed.

skan: The `MCBoundaryAlignFragment` may not emit anything, so the constructor of…
F->setAlignment(AlignBoundary);
LuoYuankeUnsubmitted
Done
ReplyInline Actions

Just to confirm that there is no MCBoundaryAlignFragment between the 2 macro fusion instructions. Right?

LuoYuanke: Just to confirm that there is no MCBoundaryAlignFragment between the 2 macro fusion…
skanAuthorUnsubmitted
Done
ReplyInline Actions

Right, there is no.

skan: Right, there is no.
F->setEmitNops(true); F->setEmitNops(true);
F->setFused(false);
} else if (NeedAlignFused && isFirstMacroFusibleInst(Inst, *MCII)) { } else if (NeedAlignFused && isFirstMacroFusibleInst(Inst, *MCII)) {
// We don't know if macro fusion happens until the reaching the next // We don't know if macro fusion happens until reaching the next
// instruction, so a place holder is put here if necessary. // instruction, so a placeholder is put here if necessary.
getOrCreateBoundaryAlignFragment(OS); OS.getOrCreateBoundaryAlignFragment();
MaskRayUnsubmitted
Done
ReplyInline Actions

If you place F->setAlignment(AlignBoundary) here, you can avoid 2 setAlignment calls in X86AsmBackend::alignBranchesBegin.

MaskRay: If you place `F->setAlignment(AlignBoundary)` here, you can avoid 2 setAlignment calls in…
skanAuthorUnsubmitted
Done
ReplyInline Actions

If a MCBoundaryAlignFragment is not used to emit anything, I prefer it doesn't set the alignment. If we place F->setAlignment(AlignBoundary) here, the operation setAlignment(AlignBoundary) is unnecessary for those MCBoundaryAlignFragments that will not emit anything.

skan: If a `MCBoundaryAlignFragment` is not used to emit anything, I prefer it doesn't set the…
MaskRayUnsubmitted
Done
ReplyInline Actions

And here.

MaskRay: And here.
skanAuthorUnsubmitted
Done
ReplyInline Actions

I think we can not remove it.

skan: I think we can not remove it.
} else if (shouldAddPrefix(Inst)) {
// Emit prefixes before instruction that doesn't need to be aligned.
auto *F = OS.getOrCreateBoundaryAlignFragment();
F->setAlignment(AlignBoundary);
F->setMaxBytesToEmit(GetRemainingPrefixSize(Inst));
F->setValue(choosePrefix(Inst));
} }
PrevInst = Inst; PrevInst = Inst;
} }
/// Insert a MCBoundaryAlignFragment to mark the end of the branch to be aligned /// Set the last fragment in the set of fragments to be aligned (which is
/// if necessary. /// current fragment indeed) for BF and insert a new BF to prevent further
/// instruction from being added to the current fragment if necessary.
void X86AsmBackend::alignBranchesEnd(MCObjectStreamer &OS, const MCInst &Inst) { void X86AsmBackend::alignBranchesEnd(MCObjectStreamer &OS, const MCInst &Inst) {
if (!needAlign(OS)) if (!needAlign(OS) || !needAlignInst(Inst))
return; return;
// If the branch is emitted into a MCRelaxableFragment, we can determine the
// size of the branch easily in MCAssembler::relaxBoundaryAlign. When the const MCFragment *CF = OS.getCurrentFragment();
MaskRayUnsubmitted
Done
ReplyInline Actions

It seems unnecessary to move it from alignBranchesBegin here.

MaskRay: It seems unnecessary to move it from alignBranchesBegin here.
skanAuthorUnsubmitted
Done
ReplyInline Actions

The purpose of moving PrevInst = Inst here is to make the early return in alignBranchesBegin simple. Otherwise, we need to write PrevInst = Inst; return in alignBranchesBegin` rather than return.

skan: The purpose of moving `PrevInst = Inst` here is to make the early return in…
// branch is fused, the fused branch(macro fusion pair) must be emitted into for (auto *F = CF; F && F != LastFragmentToBeAligned &&
// two fragments. Or when the branch is unfused, the branch must be emitted (F->hasInstructions() || isa<MCBoundaryAlignFragment>(F));
// into one fragment. The MCRelaxableFragment naturally marks the end of the F = F->getPrevNode()) {
// fused or unfused branch. // The fragments to be aligned should be in the same section with this
// Otherwise, we need to insert a MCBoundaryAlignFragment to mark the end of // fragment, and each non-BF fragment on the path from this fragment to the
// the branch. This MCBoundaryAlignFragment may be reused to emit NOP to align // fragments to be aligned must have a fixed size after finite times of
// other branch. // relaxation. Currently, the conservatively use hasInstruction to ensure
if (needAlignInst(Inst) && !isa<MCRelaxableFragment>(OS.getCurrentFragment())) // that.
OS.insert(new MCBoundaryAlignFragment(AlignBoundary)); if (auto *BF = dyn_cast<MCBoundaryAlignFragment>(F)) {
if (BF->hasEmit())
const_cast<MCBoundaryAlignFragment *>(BF)->setFragment(CF);
// There is at most one MCBoundaryAlignFragment to align one instruction
// if we only emit NOP to align instruction.
if (AlignMaxPrefixSize == 0)
break;
}
}
LastFragmentToBeAligned = CF;
// We need no further instructions can be emitted into the current fragment.
//
// If current fragment is a MCRelaxableFragment, then no more
// instructions can be pushed into since MCRelaxableFragment only holds one
// instruction.
//
// Otherwise, we need to insert a new BF to truncate the current fragment.
// This MCBoundaryAlignFragment may be reused to emit NOP or segment override
// prefix to align other instruction.
if (!isa<MCRelaxableFragment>(OS.getCurrentFragment()))
OS.insert(new MCBoundaryAlignFragment());
// Update the maximum alignment on the current section if necessary. // Update the maximum alignment on the current section if necessary.
MCSection *Sec = OS.getCurrentSectionOnly(); MCSection *Sec = OS.getCurrentSectionOnly();
if (AlignBoundary.value() > Sec->getAlignment()) if (AlignBoundary.value() > Sec->getAlignment())
Sec->setAlignment(AlignBoundary); Sec->setAlignment(AlignBoundary);
} }
Optional<MCFixupKind> X86AsmBackend::getFixupKind(StringRef Name) const { Optional<MCFixupKind> X86AsmBackend::getFixupKind(StringRef Name) const {
▲ Show 20 Lines • Show All 633 Lines • Show Last 20 Lines