<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;"><div>I like, in general, the approach of attaching the feature bits to a fragment rather than the instruction itself, which seems like significant overkill since they will almost always (especially for compiler generated code) all be the same.</div><div><br></div><div>-Jim</div><div><br></div><div>cc greg and david since they’re looking at some (semi) related issues in the ARM backend.</div><br><div><div>On Jan 5, 2014, at 6:06 AM, Craig Topper <<a href="mailto:craig.topper@gmail.com">craig.topper@gmail.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div dir="ltr">Jim, is this something you can take a look at?</div><div class="gmail_extra"><br><br><div class="gmail_quote">On Sun, Jan 5, 2014 at 4:55 AM, David Woodhouse <span dir="ltr"><<a href="mailto:dwmw2@infradead.org" target="_blank">dwmw2@infradead.org</a>></span> wrote:<br>
<blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="im">On Mon, 2013-12-23 at 23:27 +0000, David Woodhouse wrote:<br>
> On Sun, 2013-12-22 at 00:21 +0000, David Woodhouse wrote:<br>
> ><br>
> > Jörg requested on IRC that I make the feature set mandatory for any<br>
> > target that elects to use it, rather than falling back to using the<br>
> > SubtargetInfo if no override has been set in the MCInst.<br>
> ><br>
> > Here's what that would look like...<br>
><br>
> I didn't like this; it seems to make it much more cumbersome. So I came<br>
> up with an alternative version which stores the available feature bits<br>
> in the MCRelaxableFragment instead — since I think that's the only place<br>
> it *matters* and we can't just use the current bits from the<br>
> SubtargetInfo.<br>
><br>
> This is what's now in <a href="http://git.infradead.org/users/dwmw2/llvm.git" target="_blank">http://git.infradead.org/users/dwmw2/llvm.git</a> as<br>
> one of the two bug fixes that come before the bulk of the .code16 work:<br>
<br>
</div><div class="im">> @@ -36,6 +37,16 @@ public:<br>
> /// stream \p OS.<br>
> virtual void EncodeInstruction(const MCInst &Inst, raw_ostream &OS,<br>
> SmallVectorImpl<MCFixup> &Fixups) const = 0;<br>
> +<br>
> + virtual void EncodeInstruction(const MCInst &Inst, raw_ostream &OS,<br>
> + SmallVectorImpl<MCFixup> &Fixups,<br>
> + uint64_t AvailableFeatures) const {<br>
> + return EncodeInstruction(Inst, OS, Fixups);<br>
> + }<br>
> +<br>
> + /// getAvailableFeatures - Return the current feature set, for use when an<br>
> + /// instruction is re-encoded later due to relaxation.<br>
> + virtual uint64_t getAvailableFeatures() const { return 0; }<br>
> };<br>
><br>
> } // End llvm namespace<br>
<br>
</div>Incremental patch... ?<br>
<br>
From e8646d2b6bbaeade2f33888a5aee89e537f49785 Mon Sep 17 00:00:00 2001<br>
From: David Woodhouse <<a href="mailto:David.Woodhouse@intel.com">David.Woodhouse@intel.com</a>><br>
Date: Sun, 5 Jan 2014 10:33:19 +0000<br>
Subject: [PATCH] Make AvailableFeatures argument to EncodeInstruction<br>
mandatory<br>
<br>
The fix for PR18303 added a new form of MCCodeEmitter::EncodeInstruction()<br>
which takes an AvailableFeatures argument. Clean that up somewhat by making<br>
the new form mandatory and implementing it in every back end, then providing<br>
an inline version with the old prototype that just calls the real function<br>
with this.getAvailableFeatures() as the new argument.<br>
<br>
This addresses the feedback I *anticipated* getting in response to the<br>
original patch, which never actually came. Since I'm fairly clueless about<br>
C++, and especially about the collective taste of the LLVM project, I<br>
figured it was best to actually wait to be told how to do it.<br>
<br>
But in the face of resounding silence, I might as well just have a go...<br>
---<br>
include/llvm/MC/MCCodeEmitter.h | 10 +++++-----<br>
lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp | 6 ++++--<br>
lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp | 6 ++++--<br>
lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp | 6 ++++--<br>
lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp | 3 ++-<br>
lib/Target/R600/MCTargetDesc/R600MCCodeEmitter.cpp | 6 ++++--<br>
lib/Target/R600/MCTargetDesc/SIMCCodeEmitter.cpp | 6 ++++--<br>
lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp | 6 ++++--<br>
lib/Target/SystemZ/MCTargetDesc/SystemZMCCodeEmitter.cpp | 6 ++++--<br>
lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp | 11 +----------<br>
10 files changed, 36 insertions(+), 30 deletions(-)<br>
<br>
diff --git a/include/llvm/MC/MCCodeEmitter.h b/include/llvm/MC/MCCodeEmitter.h<br>
index 1b5bc50..9c041eb 100644<br>
--- a/include/llvm/MC/MCCodeEmitter.h<br>
+++ b/include/llvm/MC/MCCodeEmitter.h<br>
@@ -36,12 +36,12 @@ public:<br>
/// EncodeInstruction - Encode the given \p Inst to bytes on the output<br>
<div class="im"> /// stream \p OS.<br>
virtual void EncodeInstruction(const MCInst &Inst, raw_ostream &OS,<br>
</div>- SmallVectorImpl<MCFixup> &Fixups) const = 0;<br>
-<br>
- virtual void EncodeInstruction(const MCInst &Inst, raw_ostream &OS,<br>
<div class="im"> SmallVectorImpl<MCFixup> &Fixups,<br>
- uint64_t AvailableFeatures) const {<br>
- return EncodeInstruction(Inst, OS, Fixups);<br>
</div>+ uint64_t AvailableFeatures) const = 0;<br>
+<br>
+ inline void EncodeInstruction(const MCInst &Inst, raw_ostream &OS,<br>
+ SmallVectorImpl<MCFixup> &Fixups) {<br>
+ return EncodeInstruction(Inst, OS, Fixups, getAvailableFeatures());<br>
<div class="im"> }<br>
<br>
/// getAvailableFeatures - Return the current feature set, for use when an<br>
</div>diff --git a/lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp b/lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp<br>
index b41c566..2bb0a78 100644<br>
--- a/lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp<br>
+++ b/lib/Target/AArch64/MCTargetDesc/AArch64MCCodeEmitter.cpp<br>
@@ -121,7 +121,8 @@ public:<br>
<div class="im"><br>
<br>
void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const;<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const;<br>
<br>
template<int hasRs, int hasRt2> unsigned<br>
fixLoadStoreExclusive(const MCInst &MI, unsigned EncodedValue) const;<br>
@@ -545,7 +546,8 @@ MCCodeEmitter *llvm::createAArch64MCCodeEmitter(const MCInstrInfo &MCII,<br>
<br>
void AArch64MCCodeEmitter::<br>
<div class="im"> EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const {<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const {<br>
if (MI.getOpcode() == AArch64::TLSDESCCALL) {<br>
// This is a directive which applies an R_AARCH64_TLSDESC_CALL to the<br>
// following (BLR) instruction. It doesn't emit any code itself so it<br>
diff --git a/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp b/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp<br>
index 4382d0d..03f7556 100644<br>
--- a/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp<br>
+++ b/lib/Target/ARM/MCTargetDesc/ARMMCCodeEmitter.cpp<br>
@@ -333,7 +333,8 @@ public:<br>
<div class="im"> }<br>
<br>
void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const;<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvaiableFeatures) const;<br>
};<br>
<br>
} // end anonymous namespace<br>
@@ -1524,7 +1525,8 @@ getShiftRight64Imm(const MCInst &MI, unsigned Op,<br>
<br>
void ARMMCCodeEmitter::<br>
<div class="im"> EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const {<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const {<br>
<div class="im"> // Pseudo instructions don't get encoded.<br>
</div> const MCInstrDesc &Desc = MCII.get(MI.getOpcode());<br>
uint64_t TSFlags = Desc.TSFlags;<br>
diff --git a/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp b/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp<br>
index aad777d..9186ca6 100644<br>
--- a/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp<br>
+++ b/lib/Target/Mips/MCTargetDesc/MipsMCCodeEmitter.cpp<br>
@@ -71,7 +71,8 @@ public:<br>
<div class="im"> }<br>
<br>
void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const;<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const;<br>
<br>
// getBinaryCodeForInstr - TableGen'erated function for getting the<br>
// binary encoding for an instruction.<br>
@@ -215,7 +216,8 @@ static void LowerDextDins(MCInst& InstIn) {<br>
/// Size the instruction with Desc.getSize().<br>
void MipsMCCodeEmitter::<br>
<div class="im"> EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const<br>
{<br>
<br>
// Non-pseudo instructions that get changed for direct object<br>
diff --git a/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp b/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp<br>
index 66ebfd2..3f0a944 100644<br>
--- a/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp<br>
+++ b/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp<br>
@@ -76,7 +76,8 @@ public:<br>
uint64_t getBinaryCodeForInstr(const MCInst &MI,<br>
SmallVectorImpl<MCFixup> &Fixups) const;<br>
<div class="im"> void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const {<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const {<br>
// For fast-isel, a float COPY_TO_REGCLASS can survive this long.<br>
// It's just a nop to keep the register classes happy, so don't<br>
// generate anything.<br>
diff --git a/lib/Target/R600/MCTargetDesc/R600MCCodeEmitter.cpp b/lib/Target/R600/MCTargetDesc/R600MCCodeEmitter.cpp<br>
index dd8df65..873d3ab 100644<br>
--- a/lib/Target/R600/MCTargetDesc/R600MCCodeEmitter.cpp<br>
+++ b/lib/Target/R600/MCTargetDesc/R600MCCodeEmitter.cpp<br>
@@ -44,7 +44,8 @@ public:<br>
<br>
/// \brief Encode the instruction and write it to the OS.<br>
virtual void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
<div class="im">- SmallVectorImpl<MCFixup> &Fixups) const;<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const;<br>
<br>
/// \returns the encoding for an MCOperand.<br>
virtual uint64_t getMachineOpValue(const MCInst &MI, const MCOperand &MO,<br>
@@ -87,7 +88,8 @@ MCCodeEmitter *llvm::createR600MCCodeEmitter(const MCInstrInfo &MCII,<br>
}<br>
<br>
void R600MCCodeEmitter::EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
<div class="im">- SmallVectorImpl<MCFixup> &Fixups) const {<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const {<br>
const MCInstrDesc &Desc = MCII.get(MI.getOpcode());<br>
if (MI.getOpcode() == AMDGPU::RETURN ||<br>
MI.getOpcode() == AMDGPU::FETCH_CLAUSE ||<br>
diff --git a/lib/Target/R600/MCTargetDesc/SIMCCodeEmitter.cpp b/lib/Target/R600/MCTargetDesc/SIMCCodeEmitter.cpp<br>
index 5af8320..b08f8eb 100644<br>
--- a/lib/Target/R600/MCTargetDesc/SIMCCodeEmitter.cpp<br>
+++ b/lib/Target/R600/MCTargetDesc/SIMCCodeEmitter.cpp<br>
@@ -55,7 +55,8 @@ public:<br>
<br>
/// \breif Encode the instruction and write it to the OS.<br>
virtual void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
<div class="im">- SmallVectorImpl<MCFixup> &Fixups) const;<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const;<br>
<br>
/// \returns the encoding for an MCOperand.<br>
virtual uint64_t getMachineOpValue(const MCInst &MI, const MCOperand &MO,<br>
@@ -125,7 +126,8 @@ uint32_t SIMCCodeEmitter::getLitEncoding(const MCOperand &MO) const {<br>
}<br>
<br>
void SIMCCodeEmitter::EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
<div class="im">- SmallVectorImpl<MCFixup> &Fixups) const {<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const {<br>
<br>
uint64_t Encoding = getBinaryCodeForInstr(MI, Fixups);<br>
const MCInstrDesc &Desc = MCII.get(MI.getOpcode());<br>
diff --git a/lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp b/lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp<br>
index 641aed4..8493eb8 100644<br>
--- a/lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp<br>
+++ b/lib/Target/Sparc/MCTargetDesc/SparcMCCodeEmitter.cpp<br>
@@ -38,7 +38,8 @@ public:<br>
~SparcMCCodeEmitter() {}<br>
<div class="im"><br>
void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const;<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const;<br>
<br>
// getBinaryCodeForInstr - TableGen'erated function for getting the<br>
// binary encoding for an instruction.<br>
@@ -67,7 +68,8 @@ MCCodeEmitter *llvm::createSparcMCCodeEmitter(const MCInstrInfo &MCII,<br>
<br>
void SparcMCCodeEmitter::<br>
<div class="im"> EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const {<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const {<br>
unsigned Bits = getBinaryCodeForInstr(MI, Fixups);<br>
<br>
// Output the constant in big endian byte order.<br>
diff --git a/lib/Target/SystemZ/MCTargetDesc/SystemZMCCodeEmitter.cpp b/lib/Target/SystemZ/MCTargetDesc/SystemZMCCodeEmitter.cpp<br>
index f07ea7b..2bbf36d 100644<br>
--- a/lib/Target/SystemZ/MCTargetDesc/SystemZMCCodeEmitter.cpp<br>
+++ b/lib/Target/SystemZ/MCTargetDesc/SystemZMCCodeEmitter.cpp<br>
@@ -35,7 +35,8 @@ public:<br>
<br>
// OVerride MCCodeEmitter.<br>
virtual void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
<div class="im">- SmallVectorImpl<MCFixup> &Fixups) const<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const<br>
LLVM_OVERRIDE;<br>
<br>
private:<br>
@@ -91,7 +92,8 @@ MCCodeEmitter *llvm::createSystemZMCCodeEmitter(const MCInstrInfo &MCII,<br>
<br>
void SystemZMCCodeEmitter::<br>
<div class="im"> EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const {<br>
+ SmallVectorImpl<MCFixup> &Fixups,<br>
</div>+ uint64_t AvailableFeatures) const {<br>
uint64_t Bits = getBinaryCodeForInstr(MI, Fixups);<br>
unsigned Size = MCII.get(MI.getOpcode()).getSize();<br>
// Big-endian insertion of Size bytes.<br>
diff --git a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp<br>
index 4abd115..f31219e 100644<br>
--- a/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp<br>
+++ b/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp<br>
@@ -153,11 +153,8 @@ public:<br>
<div class="im"> uint64_t Features) const;<br>
<br>
void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div><div class="im">- SmallVectorImpl<MCFixup> &Fixups) const;<br>
</div>-<br>
- void EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
SmallVectorImpl<MCFixup> &Fixups,<br>
- uint64_t AvailableFeatures) const;<br>
+ uint64_t Features) const;<br>
<br>
uint64_t getAvailableFeatures() const;<br>
<br>
@@ -1285,12 +1282,6 @@ uint64_t X86MCCodeEmitter::getAvailableFeatures() const {<br>
<div class="im"><br>
void X86MCCodeEmitter::<br>
EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
</div>- SmallVectorImpl<MCFixup> &Fixups) const {<br>
<div class="im">- return EncodeInstruction(MI, OS, Fixups, STI.getFeatureBits());<br>
</div>-}<br>
-<br>
-void X86MCCodeEmitter::<br>
-EncodeInstruction(const MCInst &MI, raw_ostream &OS,<br>
SmallVectorImpl<MCFixup> &Fixups,<br>
<div class="im HOEnZb"> uint64_t Features) const {<br>
unsigned Opcode = MI.getOpcode();<br>
</div><span class="HOEnZb"><font color="#888888">--<br>
1.8.3.1<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
<br>
<br>
--<br>
David Woodhouse Open Source Technology Centre<br>
<a href="mailto:David.Woodhouse@intel.com">David.Woodhouse@intel.com</a> Intel Corporation<br>
</div></div><br>_______________________________________________<br>
llvm-commits mailing list<br>
<a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><br>
<a href="http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/llvm-commits</a><br>
<br></blockquote></div><br><br clear="all"><div><br></div>-- <br>~Craig
</div>
</blockquote></div><br></body></html>