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

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand marked 3 inline comments as done.
uweigand added inline comments.


================
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.
----------------
craig.topper wrote:
> I know this comment was just moved, but do you know what "the right thing" means?
"The right thing" here means to implement the select via a branch sequence.  This is what does indeed happen (usually) in the non-strict case.  It does **not** happen in the strict case, because the branch sequence has different semantics than the select (specifically, with the branch sequence only one of the STRICT_SINT_TO_FP statements would be executed, while with the select they are both executed).

Now, in this case we'd **want** the branch sequence (then we'd also have the correct semantics), but it doesn't work to emit the select sequence here and expect later passes to transform it.


================
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
----------------
craig.topper wrote:
> 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?
It does not have fpexcept **here**, since the flags all get lost somewhere.  It does have fpexcept after the next patch, where I've now added a check.

I'm not sure it makes much sense to actively check for the presence of a bug here, though.  (Also, I'm not sure how technically we'd go about to check for the beginning of the line in FileCheck ...)



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

https://reviews.llvm.org/D71840