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