Page MenuHomePhabricator

[ARM] Generate vcmp instead of vcmpe

Authored by kristof.beyls on Oct 4 2019, 6:43 AM.



Based on the discussion in,
the conclusion was reached that the ARM backend should produce vcmp instead
of vcmpe instructions by default, i.e. not be producing an Invalid Operation
exception when either arguments in a floating point compare are quiet NaNs.

In the future, after constrained floating point intrinsics for floating
point compare have been introduced, vcmpe instructions probably should be
produced for those intrinsics - depending on the exact semantics they'll
be defined to have.

This patch logically consists of the following parts:

  • Revert and, which implemented fine-tuning for when to produce vcmpe (i.e. not do it for equality comparisons). The complexity introduced by those patches isn't needed anymore if we just always produce vcmp instead. Maybe these patches need to be reintroduced again once support is needed to map potential LLVM-IR constrained floating point compare intrinsics to the ARM instruction set.
  • Simply select vcmp, instead of vcmpe, see simple changes in lib/Target/ARM/
  • Adapt lots of tests that tested for vcmpe (instead of vcmp). For all of these test, the intent of what is tested for isn't related to whether the vcmp should produce an Invalid Operation exception or not.

Fixes PR43374.

Diff Detail

Event Timeline

kristof.beyls created this revision.Oct 4 2019, 6:43 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 4 2019, 6:43 AM

Fine from my perspective. The original changes were due to some language-lawyering to fix an awkward testcase, and I can completely believe my lawyering was wrong :)

efriedma accepted this revision.Oct 4 2019, 1:08 PM
efriedma added subscribers: kpn, cameron.mcinally, efriedma.

Formally, whether we lower the "fcmp" instruction using vcmp or vcmpe shouldn't matter; the difference only affects floating point state the code isn't supposed to access anyway.

That said, I'm fine with using vcmp unconditionally, since that seems to match other targets.

This revision is now accepted and ready to land.Oct 4 2019, 1:08 PM
This revision was automatically updated to reflect the committed changes.