[PATCH] [PPC64] Properly handle the mftb instruction

[Kit Barton]

================
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");
----------------
wschmidt wrote:
> Since we are no longer considering this to be deprecated, I think we should change the name of the bit to FeatureMFTB.
OK, I will change the name to FeatureMFTB

================
Comment at: lib/Target/PowerPC/AsmParser/PPCAsmParser.cpp:1189
@@ +1188,3 @@
+    if (STI.getFeatureBits()[PPC::DeprecatedMFTB]) {
+      assert(Inst.getNumOperands() == 2 && "Expecting two operands");
+      
----------------
nemanjai wrote:
> 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?
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. 

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

http://reviews.llvm.org/D10419

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