[PATCH] D122475: [X86] Refactor X86ScalarSSEf16/32/64 with hasFP16/SSE1/SSE2. NFCI

Sat Mar 26 05:14:11 PDT 2022

pengfei added inline comments.

================
Comment at: llvm/lib/Target/X86/X86FastISel.cpp:521
   case MVT::f64:
-    if (X86ScalarSSEf32) {
+    if (HasSSE2) {
       if (IsNonTemporal && HasSSE4A)
----------------
LuoYuanke wrote:
> This fix a bug? Should previous code be X86ScalarSSEf64?
Yes and no. It's a mistake but we never have a chance to trigger it :)

================
Comment at: llvm/lib/Target/X86/X86FastISel.cpp:534
   case MVT::f64:
-    if (X86ScalarSSEf32) {
       if (IsNonTemporal && HasSSE4A)
----------------
RKSimon wrote:
> pengfei wrote:
> > This looks a bug to me, but no test is affected by this change.
> Agreed - its probably worth adding additional X87/SSE1 only test runs to fast-isel-nontemporal.ll in this patch?
We can't add them to fast-isel-nontemporal.ll due to some tests have double type in arguments.
On the other hand, we can't create a test case for this. `X86FastEmitStore` is called in 4 different places, but none of them can reach here without SSE2 enabled. One of the reason is we have checked it in `isTypeLegal`. That says we can remove the else case for both f32 and f64 and add an assert instead. But I'd like to keep it as is so that we may use it in future.

================
Comment at: llvm/lib/Target/X86/X86ISelLowering.cpp:5669

+bool X86TargetLowering::hasBitPreservingFPLogic(EVT VT) const {
+  return VT == MVT::f32 || VT == MVT::f64 || VT.isVector() ||
----------------
LuoYuanke wrote:
> Just be curious. Why move those implementation from .h file?
We can't access the member functions of `Subtarget` in .h file due to `X86Subtarget` is incomplete at that point. See https://github.com/llvm/llvm-project/blob/main/llvm/lib/Target/X86/X86ISelLowering.h#L21

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D122475