[PATCH] D15240: X86: Don't emit SAHF/LAHF for 64-bit targets unless explicitly supported

[Hans Wennborg via llvm-commits]

hans added a comment.

Thanks for the quick review!


================
Comment at: lib/Target/X86/X86.td:186
@@ -185,1 +185,3 @@
+def FeatureSAHF : SubtargetFeature<"sahf", "HasSAHF", "true",
+                                   "Support SAHF and LAHF instructions">;
 def FeatureMPX     : SubtargetFeature<"mpx", "HasMPX", "true",
----------------
jfb wrote:
> I like my naming of `HasLAHFSAHF` since it's more comprehensive.
Renamed.

I would like to keep the "sahf" name though, to match gcc's -msahf flag.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:13935
@@ -13933,1 +13934,3 @@
+  // Some 64-bit targets lack SAHF support, but they do support FCOMI.
+  assert(Subtarget->hasSAHF() && "Target doesn't support SAHF or FCOMI?");
   return DAG.getNode(X86ISD::SAHF, dl, MVT::i32, TruncSrl);
----------------
jfb wrote:
> This should be fixed before committing?
No, there is nothing to be fixed. 64-bit targets all have FCOMI, so we shouldn't be emitting SAHF for 64-bit here, and the assert shouldn't fire.

================
Comment at: lib/Target/X86/X86InstrInfo.cpp:4468
@@ -4436,2 +4467,3 @@
+		}
   }
 
----------------
jfb wrote:
> I like my format for `lib/Target/X86/X86InstrInfo.cpp` in D15239 better ☺
It does look nicer :-)

================
Comment at: lib/Target/X86/X86Subtarget.cpp:192-202
@@ -191,2 +191,13 @@
 
+#if 1
+  // XXX
+  if (!In64BitMode) {
+    if (!FullFS.empty())
+      FullFS = "+sahf," + FullFS;
+    else
+      FullFS = "+sahf";
+  }
+#endif
+
+
   // Parse features string and set the CPU.
----------------
majnemer wrote:
> Looks like some debugging code was left around?
Oops. That should teach me to rush things out before lunch :-)


http://reviews.llvm.org/D15240