This is an archive of the discontinued LLVM Phabricator instance.

[FPEnv][X86] More strict int <-> FP conversion fixes
ClosedPublic

Authored by uweigand on Dec 23 2019, 6:24 AM.

Details

Summary

I've noticed several additional problems with the int <-> FP conversion logic both in common code and in the X86 target. In particular:

  • The STRICT_FP_TO_UINT expansion emits a floating-point compare. This compare can raise exceptions and therefore needs to be a strict compare. I've made it signaling (even though quiet would also be correct) as signaling is the more usual default for an LT. This code exists both in common code and in the X86 target.
  • The STRICT_UINT_TO_FP expansion algorithm was incorrect for strict mode: it emitted two STRICT_SINT_TO_FP nodes and then used a select to choose one of the results. This can cause spurious exceptions by the STRICT_SINT_TO_FP that ends up not chosen. I've fixed the algorithm to use only a single STRICT_SINT_TO_FP instead.
  • The !isStrictFPEnabled logic in DoInstructionSelection would sometimes do the wrong thing because it calls getOperationAction using the result VT. But for some opcodes, incuding [SU]INT_TO_FP, getOperationAction needs to be called using the operand VT.
  • There was some (obsolete?) code in X86DAGToDAGISel::Select that would mutate STRICT_FP_TO_[SU]INT to non-strict versions unnecessarily.

Note that these changes (including the common code ones) affect primarily X86 codegen. @craig.topper, does this look good to you?

Diff Detail

Event Timeline

uweigand created this revision.Dec 23 2019, 6:24 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 23 2019, 6:24 AM
craig.topper added inline comments.Dec 23 2019, 10:11 AM
llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1171

I think "case" is supposed to line up with "switch" in LLVM style. Check clang-format?

llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
6161

I know this comment was just moved, but do you know what "the right thing" means?

llvm/test/CodeGen/X86/fp-intrinsics-flags.ll
32–36

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?

uweigand updated this revision to Diff 235166.Dec 23 2019, 10:58 AM
uweigand marked 3 inline comments as done.Dec 23 2019, 11:03 AM
uweigand added inline comments.
llvm/lib/CodeGen/SelectionDAG/TargetLowering.cpp
6161

"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.

llvm/test/CodeGen/X86/fp-intrinsics-flags.ll
32–36

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 ...)

This revision is now accepted and ready to land.Dec 23 2019, 11:26 AM
This revision was automatically updated to reflect the committed changes.