[PATCH] D11316: [X86] -- Fix fptoui i64 conversions for IA32 (performance and correctness)

[Mitch Bodart]

mbodart added inline comments.

================
Comment at: lib/Target/X86/X86ISelLowering.cpp:26320
@@ -26204,2 +26319,3 @@
 bool X86TargetLowering::isTargetFTOL() const {
-  return Subtarget->isTargetKnownWindowsMSVC() && !Subtarget->is64Bit();
+  // FTOL usage is currently incorrect, as it computes conversion
+  // to *signed* i64, so it is disabled here.  Given the presence
----------------
majnemer wrote:
> mkuper wrote:
> > I'm ok with just deleting the FTOL code immediately, unless anyone objects.
> > (Can be a follow-up patch)
> I think it makes sense to remove it.
I'm fine with removing it, but need a little guidance as to the depth of removal.

Certainly isIntegerTypeFTOL and isTargetFTOL, and their use in FP_TO_INTHelper, can be removed.

But further deletion of the WIN_FTOL instructions in the .td files would prevent any future use of that library routine.  How can I be certain that isn't being used, or won't be needed?


================
Comment at: test/CodeGen/X86/scalar-fp-to-i64.ll:17
@@ +16,3 @@
+;
+; RUN: llc < %s -mtriple=i386-pc-windows-msvc     -mcpu=skx      | FileCheck %s --check-prefix=CHECK --check-prefix=AVX512_32
+; RUN: llc < %s -mtriple=i386-unknown-linux-gnu   -mcpu=skx      | FileCheck %s --check-prefix=CHECK --check-prefix=AVX512_32
----------------
mkuper wrote:
> Can you use -mattr here instead of an explicit -mcpu?
I see both -mcpu and -mattr being used for skx and avx512 testing in lit tests.
-mcpu seemed simpler and more "bullet proof" if you will in that it captures
all relevant attributes (sse, sse2, ... avx, skx).

But if -mattr is the preferred method, I can certainly change to that.

What is the BKM?


================
Comment at: test/CodeGen/X86/win_ftol2.ll:4
@@ -3,3 +3,3 @@
 ; RUN: llc < %s -mtriple=i686-pc-linux | FileCheck %s -check-prefix=COMPILERRT
-; RUN: llc < %s -mtriple=x86_64-pc-win32 | FileCheck %s -check-prefix=COMPILERRT
+; R U N: llc < %s -mtriple=x86_64-pc-win32 | FileCheck %s -check-prefix=COMPILERRT
 ; RUN: llc < %s -mtriple=x86_64-pc-mingw32 | FileCheck %s -check-prefix=COMPILERRT
----------------
mkuper wrote:
> What happened to this RUN line?
Oops, forgot to reset it before submitting the review.
I was playing around with different ways to disable this test.
It's left as RUN in my current version.


http://reviews.llvm.org/D11316