[PowerPC] Split s34imm into two types

[stefanp]

Summary

Currently the instruction paddi always takes s34imm as the type for the
34 bit immediate. However, the PC Relative form of the instruction should
not produce the same fixup as the non PC Relative form.
This patch splits the s34imm type into s34imm and s34imm_pcrel so that two
different fixups can be emitted.

[kamaub]

This is LGTM, one suggestion on the clang-format change that can be done pre-commit.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp

108

I think it might be a good idea to ignore the clang-format suggestions in this case since the previous way is alot more readable.

[nemanjai]

Minor nits, otherwise LGTM.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp

108

Yes. Please do not change the existing ones. Unrelated whitespace changes are quite detrimental to meaningful git log history.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp

413

I think we can just skip the switch that does nothing and add an llvm_unreachable for this case.

llvm/lib/Target/PowerPC/PPCInstrPrefix.td

399

It seems very odd to me that we would use the _pcrel version here. There should be no way to do anything PC-relative with this instruction since it will necessarily set the PC-Rel bit to zero. The immediate should always be a real immediate (never any fixup).

So although it doesn't matter, we should probably not use the _pcrel version because it will be confusing. I was certainly confused and wrote about 3 versions of this comment :)

[nemanjai]

Forgot to select Accept.

[stefanp]

Fixed the alignment that was auto-fixed.
Removed the switch that was not needed.
Fixed the types in the TD file to be s34imm.

[stefanp]

llvm/lib/Target/PowerPC/PPCInstrPrefix.td

399

You are correct.
For some reason I added this here thinking that MLS_DForm_SI34_RT5 was a multi-class just like the one above it... Anyway, I'll fix it.

[stefanp]

Going to reopen this review.
The initial commit was pulled due to asserts being hit on an release+asserts build.

[stefanp]

Changed the way that the error is emitted.

llvm/lib/Target/PowerPC/MCTargetDesc/PPCAsmBackend.cpp

 //===-- PPCAsmBackend.cpp - PPC Assembler Backend -------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "MCTargetDesc/PPCFixupKinds.h"
static uint64_t adjustFixupValue(unsigned Kind, uint64_t Value) {
   case PPC::fixup_ppc_br24abs:
   case PPC::fixup_ppc_br24_notoc:
     return Value & 0x3fffffc;
   case PPC::fixup_ppc_half16:
     return Value & 0xffff;
   case PPC::fixup_ppc_half16ds:
     return Value & 0xfffc;
   case PPC::fixup_ppc_pcrel34:
+  case PPC::fixup_ppc_imm34:
     return Value & 0x3ffffffff;
   }
 }
 
 static unsigned getFixupKindNumBytes(unsigned Kind) {
   switch (Kind) {
   default:
     llvm_unreachable("Unknown fixup kind!");
   case FK_Data_1:
     return 1;
   case FK_Data_2:
   case PPC::fixup_ppc_half16:
   case PPC::fixup_ppc_half16ds:
     return 2;
   case FK_Data_4:
   case PPC::fixup_ppc_brcond14:
   case PPC::fixup_ppc_brcond14abs:
   case PPC::fixup_ppc_br24:
   case PPC::fixup_ppc_br24abs:
   case PPC::fixup_ppc_br24_notoc:
     return 4;
   case PPC::fixup_ppc_pcrel34:
+  case PPC::fixup_ppc_imm34:
   case FK_Data_8:
     return 8;
   case PPC::fixup_ppc_nofixup:
     return 0;
   }
 }
 
 namespace {
const static MCFixupKindInfo InfosBE[PPC::NumTargetFixupKinds] = {
       { "fixup_ppc_br24",        6,      24,   MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_ppc_br24_notoc",  6,      24,   MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_ppc_brcond14",    16,     14,   MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_ppc_br24abs",     6,      24,   0 },
       { "fixup_ppc_brcond14abs", 16,     14,   0 },
       { "fixup_ppc_half16",       0,     16,   0 },
       { "fixup_ppc_half16ds",     0,     14,   0 },
       { "fixup_ppc_pcrel34",     0,      34,   MCFixupKindInfo::FKF_IsPCRel },
+      { "fixup_ppc_imm34",       0,      34,   0 },
       { "fixup_ppc_nofixup",      0,      0,   0 }
     };
     const static MCFixupKindInfo InfosLE[PPC::NumTargetFixupKinds] = {
       // name                    offset  bits  flags
       { "fixup_ppc_br24",        2,      24,   MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_ppc_br24_notoc",  2,      24,   MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_ppc_brcond14",    2,      14,   MCFixupKindInfo::FKF_IsPCRel },
       { "fixup_ppc_br24abs",     2,      24,   0 },
       { "fixup_ppc_brcond14abs", 2,      14,   0 },
       { "fixup_ppc_half16",      0,      16,   0 },
       { "fixup_ppc_half16ds",    2,      14,   0 },
       { "fixup_ppc_pcrel34",     0,      34,   MCFixupKindInfo::FKF_IsPCRel },
+      { "fixup_ppc_imm34",       0,      34,   0 },
       { "fixup_ppc_nofixup",     0,       0,   0 }
     };
 
     // Fixup kinds from .reloc directive are like R_PPC_NONE/R_PPC64_NONE. They
     // do not require any extra processing.
     if (Kind >= FirstLiteralRelocationKind)
       return MCAsmBackend::getFixupKindInfo(FK_NONE);
 

llvm/lib/Target/PowerPC/MCTargetDesc/PPCELFObjectWriter.cpp

case PPC::fixup_ppc_nofixup:
       case MCSymbolRefExpr::VK_PPC_TLS:
         if (is64Bit())
           Type = ELF::R_PPC64_TLS;
         else
           Type = ELF::R_PPC_TLS;
         break;
       }
       break;
+    case PPC::fixup_ppc_imm34:
+      llvm_unreachable("Unsupported Modifier for fixup_ppc_imm34.");
+      break;
     case FK_Data_8:
       switch (Modifier) {
       default: llvm_unreachable("Unsupported Modifier");
       case MCSymbolRefExpr::VK_PPC_TOCBASE:
         Type = ELF::R_PPC64_TOC;
         break;
       case MCSymbolRefExpr::VK_None:
         Type = ELF::R_PPC64_ADDR64;

llvm/lib/Target/PowerPC/MCTargetDesc/PPCFixupKinds.h

enum Fixups {
 
   /// A 14-bit fixup corresponding to lo16(_foo) with implied 2 zero bits for
   /// instrs like 'std'.
   fixup_ppc_half16ds,
 
   // A 34-bit fixup corresponding to PC-relative paddi.
   fixup_ppc_pcrel34,
 
+  // A 34-bit fixup corresponding to Non-PC-relative paddi.
+  fixup_ppc_imm34,
+
   /// Not a true fixup, but ties a symbol to a call to __tls_get_addr for the
   /// TLS general and local dynamic models, or inserts the thread-pointer
   /// register number.
   fixup_ppc_nofixup,
 
   // Marker
   LastTargetFixupKind,
   NumTargetFixupKinds = LastTargetFixupKind - FirstTargetFixupKind
 };
 }
 }
 
 #endif

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.h

public:
   unsigned getAbsCondBrEncoding(const MCInst &MI, unsigned OpNo,
                                 SmallVectorImpl<MCFixup> &Fixups,
                                 const MCSubtargetInfo &STI) const;
   unsigned getImm16Encoding(const MCInst &MI, unsigned OpNo,
                             SmallVectorImpl<MCFixup> &Fixups,
                             const MCSubtargetInfo &STI) const;
   uint64_t getImm34Encoding(const MCInst &MI, unsigned OpNo,
                             SmallVectorImpl<MCFixup> &Fixups,
+                            const MCSubtargetInfo &STI,
+                            MCFixupKind Fixup) const;
+  uint64_t getImm34EncodingNoPCRel(const MCInst &MI, unsigned OpNo,
+                                   SmallVectorImpl<MCFixup> &Fixups,
+                                   const MCSubtargetInfo &STI) const;
+  uint64_t getImm34EncodingPCRel(const MCInst &MI, unsigned OpNo,
+                                 SmallVectorImpl<MCFixup> &Fixups,
                                  const MCSubtargetInfo &STI) const;
   unsigned getMemRIEncoding(const MCInst &MI, unsigned OpNo,
                             SmallVectorImpl<MCFixup> &Fixups,
                             const MCSubtargetInfo &STI) const;
   unsigned getMemRIXEncoding(const MCInst &MI, unsigned OpNo,
                              SmallVectorImpl<MCFixup> &Fixups,
                              const MCSubtargetInfo &STI) const;
   unsigned getMemRIX16Encoding(const MCInst &MI, unsigned OpNo,
                                SmallVectorImpl<MCFixup> &Fixups,

llvm/lib/Target/PowerPC/MCTargetDesc/PPCMCCodeEmitter.cpp

unsigned PPCMCCodeEmitter::getImm16Encoding(const MCInst &MI, unsigned OpNo,
   if (MO.isReg() || MO.isImm()) return getMachineOpValue(MI, MO, Fixups, STI);
 
   // Add a fixup for the immediate field.
   Fixups.push_back(MCFixup::create(IsLittleEndian? 0 : 2, MO.getExpr(),
                                    (MCFixupKind)PPC::fixup_ppc_half16));
   return 0;
 }
 
-uint64_t
-PPCMCCodeEmitter::getImm34Encoding(const MCInst &MI, unsigned OpNo,
+uint64_t PPCMCCodeEmitter::getImm34Encoding(const MCInst &MI, unsigned OpNo,
                                             SmallVectorImpl<MCFixup> &Fixups,
-                                   const MCSubtargetInfo &STI) const {
+                                            const MCSubtargetInfo &STI,
+                                            MCFixupKind Fixup) const {
   const MCOperand &MO = MI.getOperand(OpNo);
-  if (MO.isReg() || MO.isImm())
+  assert(!MO.isReg() && "Not expecting a register for this operand.");
+  if (MO.isImm())
     return getMachineOpValue(MI, MO, Fixups, STI);
 
   // Add a fixup for the immediate field.
-  Fixups.push_back(MCFixup::create(0, MO.getExpr(),
-                                   (MCFixupKind)PPC::fixup_ppc_pcrel34));
+  Fixups.push_back(MCFixup::create(0, MO.getExpr(), Fixup));
   return 0;
 }
 
+uint64_t
+PPCMCCodeEmitter::getImm34EncodingNoPCRel(const MCInst &MI, unsigned OpNo,
+                                          SmallVectorImpl<MCFixup> &Fixups,
+                                          const MCSubtargetInfo &STI) const {
+  return getImm34Encoding(MI, OpNo, Fixups, STI,
+                          (MCFixupKind)PPC::fixup_ppc_imm34);
+}
+
+uint64_t
+PPCMCCodeEmitter::getImm34EncodingPCRel(const MCInst &MI, unsigned OpNo,
+                                        SmallVectorImpl<MCFixup> &Fixups,
+                                        const MCSubtargetInfo &STI) const {
+  return getImm34Encoding(MI, OpNo, Fixups, STI,
+                          (MCFixupKind)PPC::fixup_ppc_pcrel34);
+}
+
 unsigned PPCMCCodeEmitter::getMemRIEncoding(const MCInst &MI, unsigned OpNo,
                                             SmallVectorImpl<MCFixup> &Fixups,
                                             const MCSubtargetInfo &STI) const {
   // Encode (imm, reg) as a memri, which has the low 16-bits as the
   // displacement and the next 5 bits as the register #.
   assert(MI.getOperand(OpNo+1).isReg());
   unsigned RegBits = getMachineOpValue(MI, MI.getOperand(OpNo+1), Fixups, STI) << 16;
 

llvm/lib/Target/PowerPC/PPCInstrInfo.td

 }
 def PPCS34ImmAsmOperand : AsmOperandClass {
   let Name = "S34Imm";
   let PredicateMethod = "isS34Imm";
   let RenderMethod = "addImmOperands";
 }
 def s34imm : Operand<i64> {
   let PrintMethod = "printS34ImmOperand";
-  let EncoderMethod = "getImm34Encoding";
+  let EncoderMethod = "getImm34EncodingNoPCRel";
+  let ParserMatchClass = PPCS34ImmAsmOperand;
+  let DecoderMethod = "decodeSImmOperand<34>";
+}
+def s34imm_pcrel : Operand<i64> {
+  let PrintMethod = "printS34ImmOperand";
+  let EncoderMethod = "getImm34EncodingPCRel";
   let ParserMatchClass = PPCS34ImmAsmOperand;
   let DecoderMethod = "decodeSImmOperand<34>";
 }
 def PPCImmZeroAsmOperand : AsmOperandClass {
   let Name = "ImmZero";
   let PredicateMethod = "isImmZero";
   let RenderMethod = "addImmOperands";
 }

llvm/lib/Target/PowerPC/PPCInstrPrefix.td

 
 def PrefixInstrs : Predicate<"Subtarget->hasPrefixInstrs()">;
 def IsISA3_1 : Predicate<"Subtarget->isISA3_1()">;
 
 let Predicates = [PrefixInstrs] in {
   let Interpretation64Bit = 1, isCodeGenOnly = 1 in {
     defm PADDI8 :
       MLS_DForm_R_SI34_RTA5_p<14, (outs g8rc:$RT), (ins g8rc:$RA, s34imm:$SI),
-                              (ins immZero:$RA, s34imm:$SI),
+                              (ins immZero:$RA, s34imm_pcrel:$SI),
                               "paddi $RT, $RA, $SI", IIC_LdStLFD>;
     let isReMaterializable = 1, isAsCheapAsAMove = 1, isMoveImm = 1 in {
       def PLI8 : MLS_DForm_SI34_RT5<14, (outs g8rc:$RT),
                                     (ins s34imm:$SI),
                                     "pli $RT, $SI", IIC_IntSimple, []>;
     }
   }
   defm PADDI :
     MLS_DForm_R_SI34_RTA5_p<14, (outs gprc:$RT), (ins gprc:$RA, s34imm:$SI),
-                            (ins immZero:$RA, s34imm:$SI),
+                            (ins immZero:$RA, s34imm_pcrel:$SI),
                             "paddi $RT, $RA, $SI", IIC_LdStLFD>;
   let isReMaterializable = 1, isAsCheapAsAMove = 1, isMoveImm = 1 in {
     def PLI : MLS_DForm_SI34_RT5<14, (outs gprc:$RT),
                                  (ins s34imm:$SI),
                                  "pli $RT, $SI", IIC_IntSimple, []>;
   }
 
   let mayLoad = 1, mayStore = 0 in {

llvm/test/MC/PowerPC/ppc64-errors-emit-obj.s

+# RUN: not --crash llvm-mc -triple powerpc64-- --filetype=obj < %s 2> %t
+# RUN: FileCheck < %t %s
+# RUN: not --crash llvm-mc -triple powerpc64le-- --filetype=obj < %s 2> %t
+# RUN: FileCheck < %t %s
+
+# CHECK: Unsupported Modifier for fixup_ppc_imm34.
+paddi 3, 13, symbol@toc, 0