[PATCHES] R600/SI: VI fixes
Tom Stellard
tom at stellard.net
Thu Jan 15 10:24:53 PST 2015
On Thu, Jan 15, 2015 at 05:12:03PM +0100, Marek Olšák wrote:
> Hi Tom,
>
> I reworked the patch because it contained a bug and I also needed a
> separate helper for converting pseudos to native opcodes that deals
> with the various return values nicely.
>
> Changes: I took most of the code from hasVALU32BitEncoding dealing
> with pseudos and put it into new AMDGPUInstInfo::pseudoToMCOpcode. I
> also moved all dependencies from
> AMDGPUMCInstLower to AMDGPUInstInfo. Then I updated both
> hasVALU32BitEncoding and AMDGPUMCInstLower to use the new function.
>
> On top of that, it reports an error if a pseudo cannot be converted to
> a native opcode during codegen. This can happen due to a bug, and we
> definitely don't want to emit pseudos into the binary.
>
> Please review.
>
LGTM.
> Thanks,
>
> Marek
>
>
> On Wed, Jan 14, 2015 at 12:05 AM, Tom Stellard <tom at stellard.net> wrote:
> > On Tue, Jan 13, 2015 at 05:10:32PM +0100, Marek Olšák wrote:
> >> Hi Tom and Matt,
> >>
> >> Attached is a new version of patch "R600/SI: Don't shrink instructions
> >> whose e32 encoding doesn't exist".
> >>
> >> I decided to update the hasVALU32BitEncoding function and use that
> >> instead. It also explains the difference between getMCOpcode returning
> >> -1 and (uint16_t)-1.
> >>
> >
> > LGTM.
> >
> >> Marek
> >>
> >
> >> From 9991f2e5aa099aac44a07af75a1760cdbd0b3a42 Mon Sep 17 00:00:00 2001
> >> From: =?UTF-8?q?Marek=20Ol=C5=A1=C3=A1k?= <marek.olsak at amd.com>
> >> Date: Mon, 15 Dec 2014 13:09:36 +0100
> >> Subject: [PATCH] R600/SI: Don't shrink instructions whose e32 encoding doesn't
> >> exist
> >>
> >> v2: modify hasVALU32BitEncoding instead
> >> ---
> >> lib/Target/R600/AMDGPUMCInstLower.cpp | 16 +++++++---------
> >> lib/Target/R600/AMDGPUMCInstLower.h | 7 +++----
> >> lib/Target/R600/SIInstrInfo.cpp | 14 +++++++++++++-
> >> lib/Target/R600/SIShrinkInstructions.cpp | 9 +++++----
> >> 4 files changed, 28 insertions(+), 18 deletions(-)
> >>
> >> diff --git a/lib/Target/R600/AMDGPUMCInstLower.cpp b/lib/Target/R600/AMDGPUMCInstLower.cpp
> >> index 84ee784..3d98175 100644
> >> --- a/lib/Target/R600/AMDGPUMCInstLower.cpp
> >> +++ b/lib/Target/R600/AMDGPUMCInstLower.cpp
> >> @@ -40,7 +40,7 @@ AMDGPUMCInstLower::AMDGPUMCInstLower(MCContext &ctx, const AMDGPUSubtarget &st):
> >> { }
> >>
> >> enum AMDGPUMCInstLower::SISubtarget
> >> -AMDGPUMCInstLower::AMDGPUSubtargetToSISubtarget(unsigned Gen) const {
> >> +AMDGPUMCInstLower::AMDGPUSubtargetToSISubtarget(unsigned Gen) {
> >> switch (Gen) {
> >> default:
> >> return AMDGPUMCInstLower::SI;
> >> @@ -49,19 +49,17 @@ AMDGPUMCInstLower::AMDGPUSubtargetToSISubtarget(unsigned Gen) const {
> >> }
> >> }
> >>
> >> -unsigned AMDGPUMCInstLower::getMCOpcode(unsigned MIOpcode) const {
> >> +int AMDGPUMCInstLower::getMCOpcode(unsigned MIOpcode, unsigned Gen) {
> >>
> >> - int MCOpcode = AMDGPU::getMCOpcode(MIOpcode,
> >> - AMDGPUSubtargetToSISubtarget(ST.getGeneration()));
> >> - if (MCOpcode == -1)
> >> - MCOpcode = MIOpcode;
> >> -
> >> - return MCOpcode;
> >> + return AMDGPU::getMCOpcode(MIOpcode, AMDGPUSubtargetToSISubtarget(Gen));
> >> }
> >>
> >> void AMDGPUMCInstLower::lower(const MachineInstr *MI, MCInst &OutMI) const {
> >>
> >> - OutMI.setOpcode(getMCOpcode(MI->getOpcode()));
> >> + int MIOpcode = MI->getOpcode();
> >> + int MCOpcode = getMCOpcode(MIOpcode, ST.getGeneration());
> >> +
> >> + OutMI.setOpcode(MCOpcode != -1 ? MCOpcode : MIOpcode);
> >>
> >> for (const MachineOperand &MO : MI->explicit_operands()) {
> >> MCOperand MCOp;
> >> diff --git a/lib/Target/R600/AMDGPUMCInstLower.h b/lib/Target/R600/AMDGPUMCInstLower.h
> >> index 0ae4d11..56dc3e3 100644
> >> --- a/lib/Target/R600/AMDGPUMCInstLower.h
> >> +++ b/lib/Target/R600/AMDGPUMCInstLower.h
> >> @@ -31,10 +31,7 @@ class AMDGPUMCInstLower {
> >>
> >> /// Convert a member of the AMDGPUSubtarget::Generation enum to the
> >> /// SISubtarget enum.
> >> - enum SISubtarget AMDGPUSubtargetToSISubtarget(unsigned Gen) const;
> >> -
> >> - /// Get the MC opcode for this MachineInstr.
> >> - unsigned getMCOpcode(unsigned MIOpcode) const;
> >> + static enum SISubtarget AMDGPUSubtargetToSISubtarget(unsigned Gen);
> >>
> >> public:
> >> AMDGPUMCInstLower(MCContext &ctx, const AMDGPUSubtarget &ST);
> >> @@ -42,6 +39,8 @@ public:
> >> /// \brief Lower a MachineInstr to an MCInst
> >> void lower(const MachineInstr *MI, MCInst &OutMI) const;
> >>
> >> + /// Get the MC opcode for this MachineInstr.
> >> + static int getMCOpcode(unsigned MIOpcode, unsigned Gen);
> >> };
> >>
> >> } // End namespace llvm
> >> diff --git a/lib/Target/R600/SIInstrInfo.cpp b/lib/Target/R600/SIInstrInfo.cpp
> >> index 6454ad5..4105929 100644
> >> --- a/lib/Target/R600/SIInstrInfo.cpp
> >> +++ b/lib/Target/R600/SIInstrInfo.cpp
> >> @@ -1055,7 +1055,19 @@ bool SIInstrInfo::canFoldOffset(unsigned OffsetSize, unsigned AS) const {
> >> }
> >>
> >> bool SIInstrInfo::hasVALU32BitEncoding(unsigned Opcode) const {
> >> - return AMDGPU::getVOPe32(Opcode) != -1;
> >> + int Op32 = AMDGPU::getVOPe32(Opcode);
> >> + if (Op32 == -1)
> >> + return false;
> >> +
> >> + int MCOp = AMDGPU::getMCOpcode(Op32, RI.ST.getGeneration());
> >> +
> >> + // -1 means that Op32 is already a native instruction.
> >> + if (MCOp == -1)
> >> + return true;
> >> +
> >> + // (uint16_t)-1 means that Op32 is a pseudo instruction that has
> >> + // no encoding in the given subtarget generation.
> >> + return MCOp != (uint16_t)-1;
> >> }
> >>
> >> bool SIInstrInfo::hasModifiers(unsigned Opcode) const {
> >> diff --git a/lib/Target/R600/SIShrinkInstructions.cpp b/lib/Target/R600/SIShrinkInstructions.cpp
> >> index e97f934..e8cf57b 100644
> >> --- a/lib/Target/R600/SIShrinkInstructions.cpp
> >> +++ b/lib/Target/R600/SIShrinkInstructions.cpp
> >> @@ -10,6 +10,7 @@
> >> //
> >>
> >> #include "AMDGPU.h"
> >> +#include "AMDGPUMCInstLower.h"
> >> #include "AMDGPUSubtarget.h"
> >> #include "SIInstrInfo.h"
> >> #include "llvm/ADT/Statistic.h"
> >> @@ -213,13 +214,13 @@ bool SIShrinkInstructions::runOnMachineFunction(MachineFunction &MF) {
> >> continue;
> >> }
> >>
> >> - int Op32 = AMDGPU::getVOPe32(MI.getOpcode());
> >> -
> >> - // Op32 could be -1 here if we started with an instruction that had a
> >> + // getVOPe32 could be -1 here if we started with an instruction that had
> >> // a 32-bit encoding and then commuted it to an instruction that did not.
> >> - if (Op32 == -1)
> >> + if (!TII->hasVALU32BitEncoding(MI.getOpcode()))
> >> continue;
> >>
> >> + int Op32 = AMDGPU::getVOPe32(MI.getOpcode());
> >> +
> >> if (TII->isVOPC(Op32)) {
> >> unsigned DstReg = MI.getOperand(0).getReg();
> >> if (TargetRegisterInfo::isVirtualRegister(DstReg)) {
> >> --
> >> 2.1.0
> >>
> >
> From 516b22f87bc0a143a593d1ec52f01c883337eeb0 Mon Sep 17 00:00:00 2001
> From: =?UTF-8?q?Marek=20Ol=C5=A1=C3=A1k?= <marek.olsak at amd.com>
> Date: Mon, 15 Dec 2014 13:09:36 +0100
> Subject: [PATCH] R600/SI: Don't shrink instructions whose e32 encoding doesn't
> exist
>
> v2: modify hasVALU32BitEncoding instead
> v3: - add pseudoToMCOpcode helper to AMDGPUInstInfo, which is used by both
> hasVALU32BitEncoding and AMDGPUMCInstLower::lower
> - report an error if a pseudo can't be lowered
> ---
> lib/Target/R600/AMDGPUInstrInfo.cpp | 33 +++++++++++++++++++++++++++++++-
> lib/Target/R600/AMDGPUInstrInfo.h | 5 +++++
> lib/Target/R600/AMDGPUMCInstLower.cpp | 29 +++++++++-------------------
> lib/Target/R600/AMDGPUMCInstLower.h | 14 --------------
> lib/Target/R600/SIInstrInfo.cpp | 6 +++++-
> lib/Target/R600/SIInstrInfo.h | 1 -
> lib/Target/R600/SIInstrInfo.td | 2 +-
> lib/Target/R600/SIShrinkInstructions.cpp | 9 +++++----
> 8 files changed, 57 insertions(+), 42 deletions(-)
>
> diff --git a/lib/Target/R600/AMDGPUInstrInfo.cpp b/lib/Target/R600/AMDGPUInstrInfo.cpp
> index 5beaa68..e34a7b7 100644
> --- a/lib/Target/R600/AMDGPUInstrInfo.cpp
> +++ b/lib/Target/R600/AMDGPUInstrInfo.cpp
> @@ -341,8 +341,39 @@ int AMDGPUInstrInfo::getMaskedMIMGOp(uint16_t Opcode, unsigned Channels) const {
> // instead.
> namespace llvm {
> namespace AMDGPU {
> -int getMCOpcode(uint16_t Opcode, unsigned Gen) {
> +static int getMCOpcode(uint16_t Opcode, unsigned Gen) {
> return getMCOpcodeGen(Opcode, (enum Subtarget)Gen);
> }
> }
> }
> +
> +// This must be kept in sync with the SISubtarget class in SIInstrInfo.td
> +enum SISubtarget {
> + SI = 0,
> + VI = 1
> +};
> +
> +enum SISubtarget AMDGPUSubtargetToSISubtarget(unsigned Gen) {
> + switch (Gen) {
> + default:
> + return SI;
> + case AMDGPUSubtarget::VOLCANIC_ISLANDS:
> + return VI;
> + }
> +}
> +
> +int AMDGPUInstrInfo::pseudoToMCOpcode(int Opcode) const {
> + int MCOp = AMDGPU::getMCOpcode(Opcode,
> + AMDGPUSubtargetToSISubtarget(RI.ST.getGeneration()));
> +
> + // -1 means that Opcode is already a native instruction.
> + if (MCOp == -1)
> + return Opcode;
> +
> + // (uint16_t)-1 means that Opcode is a pseudo instruction that has
> + // no encoding in the given subtarget generation.
> + if (MCOp == (uint16_t)-1)
> + return -1;
> +
> + return MCOp;
> +}
> diff --git a/lib/Target/R600/AMDGPUInstrInfo.h b/lib/Target/R600/AMDGPUInstrInfo.h
> index da9833d..e28ce0f 100644
> --- a/lib/Target/R600/AMDGPUInstrInfo.h
> +++ b/lib/Target/R600/AMDGPUInstrInfo.h
> @@ -135,6 +135,11 @@ public:
> bool isRegisterStore(const MachineInstr &MI) const;
> bool isRegisterLoad(const MachineInstr &MI) const;
>
> + /// \brief Return a target-specific opcode if Opcode is a pseudo instruction.
> + /// Return -1 if the target-specific opcode for the pseudo instruction does
> + /// not exist. If Opcode is not a pseudo instruction, this is identity.
> + int pseudoToMCOpcode(int Opcode) const;
> +
> //===---------------------------------------------------------------------===//
> // Pure virtual funtions to be implemented by sub-classes.
> //===---------------------------------------------------------------------===//
> diff --git a/lib/Target/R600/AMDGPUMCInstLower.cpp b/lib/Target/R600/AMDGPUMCInstLower.cpp
> index 84ee784..5c090e5 100644
> --- a/lib/Target/R600/AMDGPUMCInstLower.cpp
> +++ b/lib/Target/R600/AMDGPUMCInstLower.cpp
> @@ -22,6 +22,7 @@
> #include "llvm/CodeGen/MachineBasicBlock.h"
> #include "llvm/CodeGen/MachineInstr.h"
> #include "llvm/IR/Constants.h"
> +#include "llvm/IR/Function.h"
> #include "llvm/IR/GlobalVariable.h"
> #include "llvm/MC/MCCodeEmitter.h"
> #include "llvm/MC/MCContext.h"
> @@ -39,29 +40,17 @@ AMDGPUMCInstLower::AMDGPUMCInstLower(MCContext &ctx, const AMDGPUSubtarget &st):
> Ctx(ctx), ST(st)
> { }
>
> -enum AMDGPUMCInstLower::SISubtarget
> -AMDGPUMCInstLower::AMDGPUSubtargetToSISubtarget(unsigned Gen) const {
> - switch (Gen) {
> - default:
> - return AMDGPUMCInstLower::SI;
> - case AMDGPUSubtarget::VOLCANIC_ISLANDS:
> - return AMDGPUMCInstLower::VI;
> - }
> -}
> -
> -unsigned AMDGPUMCInstLower::getMCOpcode(unsigned MIOpcode) const {
> -
> - int MCOpcode = AMDGPU::getMCOpcode(MIOpcode,
> - AMDGPUSubtargetToSISubtarget(ST.getGeneration()));
> - if (MCOpcode == -1)
> - MCOpcode = MIOpcode;
> +void AMDGPUMCInstLower::lower(const MachineInstr *MI, MCInst &OutMI) const {
>
> - return MCOpcode;
> -}
> + int MCOpcode = ST.getInstrInfo()->pseudoToMCOpcode(MI->getOpcode());
>
> -void AMDGPUMCInstLower::lower(const MachineInstr *MI, MCInst &OutMI) const {
> + if (MCOpcode == -1) {
> + LLVMContext &C = MI->getParent()->getParent()->getFunction()->getContext();
> + C.emitError("AMDGPUMCInstLower::lower - Pseudo instruction doesn't have "
> + "a target-specific version: " + Twine(MI->getOpcode()));
> + }
>
> - OutMI.setOpcode(getMCOpcode(MI->getOpcode()));
> + OutMI.setOpcode(MCOpcode);
>
> for (const MachineOperand &MO : MI->explicit_operands()) {
> MCOperand MCOp;
> diff --git a/lib/Target/R600/AMDGPUMCInstLower.h b/lib/Target/R600/AMDGPUMCInstLower.h
> index 0ae4d11..d322fe0 100644
> --- a/lib/Target/R600/AMDGPUMCInstLower.h
> +++ b/lib/Target/R600/AMDGPUMCInstLower.h
> @@ -19,23 +19,9 @@ class MCContext;
> class MCInst;
>
> class AMDGPUMCInstLower {
> -
> - // This must be kept in sync with the SISubtarget class in SIInstrInfo.td
> - enum SISubtarget {
> - SI = 0,
> - VI = 1
> - };
> -
> MCContext &Ctx;
> const AMDGPUSubtarget &ST;
>
> - /// Convert a member of the AMDGPUSubtarget::Generation enum to the
> - /// SISubtarget enum.
> - enum SISubtarget AMDGPUSubtargetToSISubtarget(unsigned Gen) const;
> -
> - /// Get the MC opcode for this MachineInstr.
> - unsigned getMCOpcode(unsigned MIOpcode) const;
> -
> public:
> AMDGPUMCInstLower(MCContext &ctx, const AMDGPUSubtarget &ST);
>
> diff --git a/lib/Target/R600/SIInstrInfo.cpp b/lib/Target/R600/SIInstrInfo.cpp
> index 6454ad5..970dbd6 100644
> --- a/lib/Target/R600/SIInstrInfo.cpp
> +++ b/lib/Target/R600/SIInstrInfo.cpp
> @@ -1055,7 +1055,11 @@ bool SIInstrInfo::canFoldOffset(unsigned OffsetSize, unsigned AS) const {
> }
>
> bool SIInstrInfo::hasVALU32BitEncoding(unsigned Opcode) const {
> - return AMDGPU::getVOPe32(Opcode) != -1;
> + int Op32 = AMDGPU::getVOPe32(Opcode);
> + if (Op32 == -1)
> + return false;
> +
> + return pseudoToMCOpcode(Op32) != -1;
> }
>
> bool SIInstrInfo::hasModifiers(unsigned Opcode) const {
> diff --git a/lib/Target/R600/SIInstrInfo.h b/lib/Target/R600/SIInstrInfo.h
> index f766dc8..28cd27d 100644
> --- a/lib/Target/R600/SIInstrInfo.h
> +++ b/lib/Target/R600/SIInstrInfo.h
> @@ -325,7 +325,6 @@ namespace AMDGPU {
> int getVOPe32(uint16_t Opcode);
> int getCommuteRev(uint16_t Opcode);
> int getCommuteOrig(uint16_t Opcode);
> - int getMCOpcode(uint16_t Opcode, unsigned Gen);
> int getAddr64Inst(uint16_t Opcode);
> int getAtomicRetOp(uint16_t Opcode);
> int getAtomicNoRetOp(uint16_t Opcode);
> diff --git a/lib/Target/R600/SIInstrInfo.td b/lib/Target/R600/SIInstrInfo.td
> index dd1449d..8274d91 100644
> --- a/lib/Target/R600/SIInstrInfo.td
> +++ b/lib/Target/R600/SIInstrInfo.td
> @@ -57,7 +57,7 @@ class sopk <bits<5> si, bits<5> vi = si> {
> }
>
> // Execpt for the NONE field, this must be kept in sync with the SISubtarget enum
> -// in AMDGPUMCInstLower.h
> +// in AMDGPUInstrInfo.cpp
> def SISubtarget {
> int NONE = -1;
> int SI = 0;
> diff --git a/lib/Target/R600/SIShrinkInstructions.cpp b/lib/Target/R600/SIShrinkInstructions.cpp
> index e97f934..e8cf57b 100644
> --- a/lib/Target/R600/SIShrinkInstructions.cpp
> +++ b/lib/Target/R600/SIShrinkInstructions.cpp
> @@ -10,6 +10,7 @@
> //
>
> #include "AMDGPU.h"
> +#include "AMDGPUMCInstLower.h"
> #include "AMDGPUSubtarget.h"
> #include "SIInstrInfo.h"
> #include "llvm/ADT/Statistic.h"
> @@ -213,13 +214,13 @@ bool SIShrinkInstructions::runOnMachineFunction(MachineFunction &MF) {
> continue;
> }
>
> - int Op32 = AMDGPU::getVOPe32(MI.getOpcode());
> -
> - // Op32 could be -1 here if we started with an instruction that had a
> + // getVOPe32 could be -1 here if we started with an instruction that had
> // a 32-bit encoding and then commuted it to an instruction that did not.
> - if (Op32 == -1)
> + if (!TII->hasVALU32BitEncoding(MI.getOpcode()))
> continue;
>
> + int Op32 = AMDGPU::getVOPe32(MI.getOpcode());
> +
> if (TII->isVOPC(Op32)) {
> unsigned DstReg = MI.getOperand(0).getReg();
> if (TargetRegisterInfo::isVirtualRegister(DstReg)) {
> --
> 2.1.0
>
More information about the llvm-commits
mailing list