[PATCH] D71840: [FPEnv][X86] More strict int <-> FP conversion fixes

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp:1171
+        switch (Node->getOpcode()) {
+          case ISD::STRICT_SINT_TO_FP:
+          case ISD::STRICT_UINT_TO_FP:
----------------
I think "case" is supposed to line up with "switch" in LLVM style. Check clang-format?


================
Comment at: llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp:6161
+    // select.  We happen to get lucky and machinesink does the right
+    // thing most of the time.  This would be a good candidate for a
+    // pseudo-op, or, even better, for whole-function isel.
----------------
I know this comment was just moved, but do you know what "the right thing" means?


================
Comment at: llvm/test/CodeGen/X86/fp-intrinsics-flags.ll:32
 ; CHECK: [[MOVSDrm_alt1:%[0-9]+]]:fr64 = MOVSDrm_alt $noreg, 1, $noreg, %const.0, $noreg :: (load 8 from constant-pool)
-; CHECK: [[CMPSDrr:%[0-9]+]]:fr64 = CMPSDrr [[MOVSDrm_alt]], [[MOVSDrm_alt1]], 1, implicit $mxcsr
-; CHECK: [[COPY:%[0-9]+]]:vr128 = COPY [[CMPSDrr]]
-; CHECK: [[COPY1:%[0-9]+]]:vr128 = COPY [[MOVSDrm_alt1]]
-; CHECK: [[PANDNrr:%[0-9]+]]:vr128 = PANDNrr [[COPY]], killed [[COPY1]]
-; CHECK: [[COPY2:%[0-9]+]]:fr64 = COPY [[PANDNrr]]
-; CHECK: [[SUBSDrr:%[0-9]+]]:fr64 = SUBSDrr [[MOVSDrm_alt]], killed [[COPY2]], implicit $mxcsr
+; CHECK: COMISDrr [[MOVSDrm_alt1]], [[MOVSDrm_alt]], implicit-def $eflags, implicit $mxcsr
+; CHECK: [[FsFLD0SD:%[0-9]+]]:fr64 = FsFLD0SD
----------------
Does this instruction have fpexcept on it? Or will it after your next patch? If its not there in this patch should this check that its the beginning of the line?


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

https://reviews.llvm.org/D71840