[PPC64] Properly handle the mftb instruction

[kbarton]

Summary

The mftb instruction was incorrectly marked as deprecated in the PPC Backend. Instead, it should not be treated as deprecated, but rather be implemented using the mfspr instruction. A similar patch was put into GCC last year. Details can be found at:

https://sourceware.org/ml/binutils/2014-11/msg00383.html.

This change will replace instances of the mftb instruction with the mfspr instruction for all CPUs except 601 and pwr3. This will also be the default behaviour.

Additional details can be found in:

https://llvm.org/bugs/show_bug.cgi?id=23680

[nemanjai]

lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp

1189

I am probably missing something since the test case you have does test the mnemonic with a single operand, but I'm curious how come we don't throw this assert when processing lines 50 and 61 in the test case. Does the immediate operand get added to the DAG before this?

1195

I'm just curious about why the temp is needed. From the standpoint of readability, it would seem to make sense to call

Inst.setOpcode(PPC::MFSPR);

then the remaining modification and copy-assignment is not necessary. Unless of course there is a reason the temporary is needed (i.e. Inst is of a type that does not have setOpcode, etc.).

lib/Target/PowerPC/PPC.td

250

This is really nit-picking, but leaving the name the same seems misleading. We are essentially "un-deprecating" the instruction and adding the DeprecatedMFTB to almost every processor.
I do realize that the instruction itself will not be emitted, but a different mnemonic, but this may not be clear to someone looking at this 5 years from now.

[wschmidt]

Looks good to me with identified changes.

lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp

1188

Since we are no longer considering this to be deprecated, I think we should change the name of the bit to FeatureMFTB.

1195

Agree with Nemanja here.

lib/Target/PowerPC/PPC.td

250

Ah, I see Nemanja had the same concern.

[kbarton]

lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp

1188

OK, I will change the name to FeatureMFTB

1189

This is handled by the instruction aliases that were added for the mftb instruction.
I added the assert initially to ensure that was happening correctly, and then decided to leave it since I don't think it will hurt anything. If for some reason those aliases are removed, this code will fail somehow (hopefully at compile time) because you are accessing an operand that doesn't exist. Worst case that access is allowed, somehow, and we end up with incorrect code generated. The assert should prevent identify all of those cases.

1195

I'm following the convention that was used in all the other cases. I don't know if there are side effects of changing operands in place. I don't really see the disadvantage of doing it this way - the extra copy introduced by the temp is negligible in the grand scheme of things.

[kbarton]

Committed revision 239827.

lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp

case PPC::RLWNMobm: {
     TmpInst.addOperand(Inst.getOperand(0));
     TmpInst.addOperand(Inst.getOperand(1));
     TmpInst.addOperand(Inst.getOperand(2));
     TmpInst.addOperand(MCOperand::createImm(MB));
     TmpInst.addOperand(MCOperand::createImm(ME));
     Inst = TmpInst;
     break;
   }
+  case PPC::MFTB: {
+    if (STI.getFeatureBits()[PPC::DeprecatedMFTB]) {
+      assert(Inst.getNumOperands() == 2 && "Expecting two operands");
+      
+      MCInst TmpInst;
+      TmpInst.setOpcode(PPC::MFSPR);
+      TmpInst.addOperand(Inst.getOperand(0));
+      TmpInst.addOperand(Inst.getOperand(1));
+      Inst = TmpInst;
+    }
+    break;
+  }
   }
 }
 
 bool PPCAsmParser::MatchAndEmitInstruction(SMLoc IDLoc, unsigned &Opcode,
                                            OperandVector &Operands,
                                            MCStreamer &Out, uint64_t &ErrorInfo,
                                            bool MatchingInlineAsm) {
   MCInst Inst;

lib/Target/PowerPC/PPC.td

 include "PPCRegisterInfo.td"
 include "PPCSchedule.td"
 include "PPCInstrInfo.td"
 
 //===----------------------------------------------------------------------===//
 // PowerPC processors supported.
 //
 
-def : Processor<"generic", G3Itineraries, [Directive32]>;
+def : Processor<"generic", G3Itineraries, [Directive32, DeprecatedMFTB]>;
 def : ProcessorModel<"440", PPC440Model, [Directive440, FeatureISEL,
                                           FeatureFRES, FeatureFRSQRTE,
                                           FeatureICBT, FeatureBookE, 
                                           FeatureMSYNC, DeprecatedMFTB]>;
 def : ProcessorModel<"450", PPC440Model, [Directive440, FeatureISEL,
                                           FeatureFRES, FeatureFRSQRTE,
                                           FeatureICBT, FeatureBookE, 
                                           FeatureMSYNC, DeprecatedMFTB]>;
 def : Processor<"601", G3Itineraries, [Directive601]>;
-def : Processor<"602", G3Itineraries, [Directive602]>;
+def : Processor<"602", G3Itineraries, [Directive602,
+                                       DeprecatedMFTB]>;
 def : Processor<"603", G3Itineraries, [Directive603,
-                                       FeatureFRES, FeatureFRSQRTE]>;
+                                       FeatureFRES, FeatureFRSQRTE,
+                                       DeprecatedMFTB]>;
 def : Processor<"603e", G3Itineraries, [Directive603,
-                                        FeatureFRES, FeatureFRSQRTE]>;
+                                        FeatureFRES, FeatureFRSQRTE,
+                                        DeprecatedMFTB]>;
 def : Processor<"603ev", G3Itineraries, [Directive603,
-                                         FeatureFRES, FeatureFRSQRTE]>;
+                                         FeatureFRES, FeatureFRSQRTE,
+                                         DeprecatedMFTB]>;
 def : Processor<"604", G3Itineraries, [Directive604,
-                                       FeatureFRES, FeatureFRSQRTE]>;
+                                       FeatureFRES, FeatureFRSQRTE,
+                                       DeprecatedMFTB]>;
 def : Processor<"604e", G3Itineraries, [Directive604,
-                                        FeatureFRES, FeatureFRSQRTE]>;
+                                        FeatureFRES, FeatureFRSQRTE,
+                                        DeprecatedMFTB]>;
 def : Processor<"620", G3Itineraries, [Directive620,
-                                       FeatureFRES, FeatureFRSQRTE]>;
+                                       FeatureFRES, FeatureFRSQRTE,
+                                       DeprecatedMFTB]>;
 def : Processor<"750", G4Itineraries, [Directive750,
-                                       FeatureFRES, FeatureFRSQRTE]>;
+                                       FeatureFRES, FeatureFRSQRTE,
+                                       DeprecatedMFTB]>;
 def : Processor<"g3", G3Itineraries, [Directive750,
-                                      FeatureFRES, FeatureFRSQRTE]>;
+                                      FeatureFRES, FeatureFRSQRTE,
+                                      DeprecatedMFTB]>;
 def : Processor<"7400", G4Itineraries, [Directive7400, FeatureAltivec,
-                                        FeatureFRES, FeatureFRSQRTE]>;
+                                        FeatureFRES, FeatureFRSQRTE,
+                                        DeprecatedMFTB]>;
 def : Processor<"g4", G4Itineraries, [Directive7400, FeatureAltivec,
-                                      FeatureFRES, FeatureFRSQRTE]>;
+                                      FeatureFRES, FeatureFRSQRTE,
+                                      DeprecatedMFTB]>;
 def : Processor<"7450", G4PlusItineraries, [Directive7400, FeatureAltivec,
-                                            FeatureFRES, FeatureFRSQRTE]>;
+                                            FeatureFRES, FeatureFRSQRTE,
+                                            DeprecatedMFTB]>;
 def : Processor<"g4+", G4PlusItineraries, [Directive7400, FeatureAltivec,
-                                           FeatureFRES, FeatureFRSQRTE]>;
+                                           FeatureFRES, FeatureFRSQRTE, 
+                                           DeprecatedMFTB]>;
 
 def : ProcessorModel<"970", G5Model,
                   [Directive970, FeatureAltivec,
                    FeatureMFOCRF, FeatureFSqrt,
                    FeatureFRES, FeatureFRSQRTE, FeatureSTFIWX,
-                   Feature64Bit /*, Feature64BitRegs */]>;
+                   Feature64Bit /*, Feature64BitRegs */,
+                   DeprecatedMFTB]>;
 def : ProcessorModel<"g5", G5Model,
                   [Directive970, FeatureAltivec,
                    FeatureMFOCRF, FeatureFSqrt, FeatureSTFIWX,
                    FeatureFRES, FeatureFRSQRTE,
                    Feature64Bit /*, Feature64BitRegs */,
                    DeprecatedMFTB, DeprecatedDST]>;
 def : ProcessorModel<"e500mc", PPCE500mcModel,
                   [DirectiveE500mc, FeatureMFOCRF,
def : ProcessorModel<"a2q", PPCA2Model,
                /*, Feature64BitRegs */, FeatureQPX, DeprecatedMFTB]>;
 def : ProcessorModel<"pwr3", G5Model,
                   [DirectivePwr3, FeatureAltivec,
                    FeatureFRES, FeatureFRSQRTE, FeatureMFOCRF,
                    FeatureSTFIWX, Feature64Bit]>;
 def : ProcessorModel<"pwr4", G5Model,
                   [DirectivePwr4, FeatureAltivec, FeatureMFOCRF,
                    FeatureFSqrt, FeatureFRES, FeatureFRSQRTE,
-                   FeatureSTFIWX, Feature64Bit]>;
+                   FeatureSTFIWX, Feature64Bit, DeprecatedMFTB]>;
 def : ProcessorModel<"pwr5", G5Model,
                   [DirectivePwr5, FeatureAltivec, FeatureMFOCRF,
                    FeatureFSqrt, FeatureFRE, FeatureFRES,
                    FeatureFRSQRTE, FeatureFRSQRTES,
                    FeatureSTFIWX, Feature64Bit,
                    DeprecatedMFTB, DeprecatedDST]>;
 def : ProcessorModel<"pwr5x", G5Model,
                   [DirectivePwr5x, FeatureAltivec, FeatureMFOCRF,
def : ProcessorModel<"pwr6x", G5Model,
                   [DirectivePwr5x, FeatureAltivec, FeatureMFOCRF,
                    FeatureFCPSGN, FeatureFSqrt, FeatureFRE, FeatureFRES,
                    FeatureFRSQRTE, FeatureFRSQRTES, FeatureRecipPrec,
                    FeatureSTFIWX, FeatureLFIWAX, FeatureCMPB,
                    FeatureFPRND, Feature64Bit,
                    DeprecatedMFTB, DeprecatedDST]>;
 def : ProcessorModel<"pwr7", P7Model, ProcessorFeatures.Power7FeatureList>;
 def : ProcessorModel<"pwr8", P8Model, ProcessorFeatures.Power8FeatureList>;
-def : Processor<"ppc", G3Itineraries, [Directive32]>;
+def : Processor<"ppc", G3Itineraries, [Directive32, DeprecatedMFTB]>;
 def : ProcessorModel<"ppc64", G5Model,
                   [Directive64, FeatureAltivec,
                    FeatureMFOCRF, FeatureFSqrt, FeatureFRES,
                    FeatureFRSQRTE, FeatureSTFIWX,
-                   Feature64Bit /*, Feature64BitRegs */]>;
+                   Feature64Bit /*, Feature64BitRegs */,
+                   DeprecatedMFTB]>;
 def : ProcessorModel<"ppc64le", P8Model, ProcessorFeatures.Power8FeatureList>;
 
 //===----------------------------------------------------------------------===//
 // Calling Conventions
 //===----------------------------------------------------------------------===//
 
 include "PPCCallingConv.td"
 

lib/Target/PowerPC/PPCInstrInfo.td

 //
 
 def MFSPR : XFXForm_1<31, 339, (outs gprc:$RT), (ins i32imm:$SPR),
                       "mfspr $RT, $SPR", IIC_SprMFSPR>;
 def MTSPR : XFXForm_1<31, 467, (outs), (ins i32imm:$SPR, gprc:$RT),
                       "mtspr $SPR, $RT", IIC_SprMTSPR>;
 
 def MFTB : XFXForm_1<31, 371, (outs gprc:$RT), (ins i32imm:$SPR),
-                     "mftb $RT, $SPR", IIC_SprMFTB>, Deprecated<DeprecatedMFTB>;
+                     "mftb $RT, $SPR", IIC_SprMFTB>;
 
 // A pseudo-instruction used to implement the read of the 64-bit cycle counter
 // on a 32-bit target.
 let hasSideEffects = 1, usesCustomInserter = 1 in
 def ReadTB : Pseudo<(outs gprc:$lo, gprc:$hi), (ins),
                     "#ReadTB", []>;
 
 let Uses = [CTR] in {

test/CodeGen/PowerPC/mftb.ll

+; Check handling of the mftb instruction.
+; For CPUs 601 and pwr3, the mftb instruction should be emitted.
+; On all other CPUs (including generic, ppc, ppc64), the mfspr instruction 
+; should be used instead. There should no longer be a deprecated warning 
+; message emittedfor this instruction for any CPU.
+
+; RUN: llc -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr7 < %s 2>&1 \
+; RUN:    | FileCheck %s --check-prefix=CHECK-MFSPR
+; RUN: llc -mtriple=powerpc64-unknown-linux-gnu -mcpu=pwr8 < %s 2>&1 \
+; RUN:    | FileCheck %s --check-prefix=CHECK-MFSPR
+; RUN: llc -mtriple=powerpc64le-unknown-linux-gnu < %s 2>&1 \
+; RUN:    | FileCheck %s --check-prefix=CHECK-MFSPR
+; RUN: llc -mtriple=powerpc-unknown-linux-gnu  < %s 2>&1 \
+; RUN:    | FileCheck %s --check-prefix=CHECK-MFSPR
+; RUN: llc -mtriple=powerpc-unknown-linux-gnu -mcpu=ppc < %s 2>&1 \
+; RUN:    | FileCheck %s --check-prefix=CHECK-MFSPR
+; RUN: llc -mtriple=powerpc-unknown-linux-gnu -mcpu=601 < %s 2>&1 \
+; RUN:    | FileCheck %s --check-prefix=CHECK-MFTB
+; RUN: llc -mtriple=powerpc-unknown-linux-gnu -mcpu=pwr3 < %s 2>&1 \
+; RUN:    | FileCheck %s --check-prefix=CHECK-MFTB
+
+; CHECK-MFSPR-NOT: warning: deprecated
+; CHECK-MFTB-NOT: warning: deprecated
+
+define i32 @get_time() {
+       %time = call i32 asm "mftb $0, 268", "=r"()
+       ret i32 %time
+; CHECK-MFSPR-LABEL: @get_time
+; CHECK-MFSPR: mfspr 3, 268
+; CHECK-MFSPR: blr
+
+; CHECK-MFTB-LABEL: @get_time
+; CHECK-MFTB: mftb 3, 268
+; CHECK-MFTB: blr
+}
+
+define i32 @get_timeu() {
+       %time = call i32 asm "mftb $0, 269", "=r"()
+       ret i32 %time
+; CHECK-MFSPR-LABEL: @get_timeu
+; CHECK-MFSPR: mfspr 3, 269
+; CHECK-MFSPR: blr
+
+; CHECK-MFTB-LABEL: @get_timeu
+; CHECK-MFTB: mftbu 3
+; CHECK-MFTB: blr
+}
+
+define i32 @get_time_e() {
+       %time = call i32 asm "mftb $0", "=r"()
+       ret i32 %time
+; CHECK-MFSPR-LABEL: @get_time_e
+; CHECK-MFSPR: mfspr 3, 268
+; CHECK-MFSPR: blr
+
+; CHECK-MFTB-LABEL: @get_time_e
+; CHECK-MFTB: mftb 3, 268
+; CHECK-MFTB: blr
+}
+
+define i32 @get_timeu_e() {
+       %time = call i32 asm "mftbu $0", "=r"()
+       ret i32 %time
+; CHECK-MFSPR-LABEL: @get_timeu_e
+; CHECK-MFSPR: mfspr 3, 269
+; CHECK-MFSPR: blr
+
+; CHECK-MFTB-LABEL: @get_timeu_e
+; CHECK-MFTB: mftbu 3
+; CHECK-MFTB: blr
+}
+

test/MC/PowerPC/deprecated-p7.s

 # RUN: llvm-mc -triple powerpc64-unknown-linux-gnu -mcpu=pwr7 -show-encoding < %s 2>&1 | FileCheck %s
 # RUN: llvm-mc -triple powerpc64le-unknown-linux-gnu -mcpu=pwr7 -show-encoding < %s 2>&1 | FileCheck %s
 # RUN: llvm-mc -triple powerpc-unknown-linux-gnu -mcpu=601 -show-encoding < %s 2>&1 | FileCheck -check-prefix=CHECK-OLD %s
 
          mftb 3
-# CHECK: warning: deprecated
-# CHECK: mftb 3
+# CHECK-NOT: warning: deprecated
+# CHECK: mfspr 3, 268
 
 # CHECK-OLD-NOT: warning: deprecated
 # CHECK-OLD: mftb 3
 
 # FIXME: Test dst and friends once we can parse them.