[PATCH] D86079: [X86] Improved lowering for saturating float to int.

[Bevin Hansson via Phabricator via llvm-commits]

ebevhan added inline comments.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:281
 
+  if (Subtarget.hasSSE1() || Subtarget.hasSSE2()) {
+    // Custom lowering for saturating float to int conversions.
----------------
craig.topper wrote:
> craig.topper wrote:
> > hasSSE2() implies hasSSE1() so you don't need to check both.
> > 
> > If you want this to work for f32 SSE1 only targets. You'll need to explicitly check hasSSE2 in the Lower function. The default CPUs on Linux, Windows, and Mac all have SSE2 enabled. So I don't know if you want to support SSE1 only or not. Our test coverage for SSE1 only isn't great.
> Sorry I wrote that before I saw your comment and I didn't check the Lowering function.
Hm, okay. The tests for this also only use sse2, so perhaps that's safer.


================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:21068
+  // anything else.
+  if (SrcVT != MVT::f32 && SrcVT != MVT::f64) {
+    return SDValue();
----------------
craig.topper wrote:
> craig.topper wrote:
> > Drop curly braces
> You can use isScalarFPTypeInSSEReg helper function to check the type and SSE level
That's better, thanks.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D86079/new/

https://reviews.llvm.org/D86079