[PATCH] [PPC64] Properly handle the mftb instruction

[Bill Schmidt]

Looks good to me with identified changes.


================
Comment at: lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1188
@@ +1187,3 @@
+  case PPC::MFTB: {
+    if (STI.getFeatureBits()[PPC::DeprecatedMFTB]) {
+      assert(Inst.getNumOperands() == 2 && "Expecting two operands");
----------------
Since we are no longer considering this to be deprecated, I think we should change the name of the bit to FeatureMFTB.

================
Comment at: lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1195
@@ +1194,3 @@
+      TmpInst.addOperand(Inst.getOperand(1));
+      Inst = TmpInst;
+    }
----------------
nemanjai wrote:
> 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.).
Agree with Nemanja here.

================
Comment at: lib/Target/PowerPC/PPC.td:250
@@ -249,3 +249,3 @@
 
-def : Processor<"generic", G3Itineraries, [Directive32]>;
+def : Processor<"generic", G3Itineraries, [Directive32, DeprecatedMFTB]>;
 def : ProcessorModel<"440", PPC440Model, [Directive440, FeatureISEL,
----------------
nemanjai wrote:
> 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.
Ah, I see Nemanja had the same concern.

http://reviews.llvm.org/D10419

EMAIL PREFERENCES
  http://reviews.llvm.org/settings/panel/emailpreferences/