[PATCH] D70226: Add an option to disable strict float node mutating to an normal float node

[Ulrich Weigand via Phabricator via llvm-commits]

uweigand added a comment.

Thanks!   I just noticed there are two more places where we need to be more careful when strict FP mode is enforced:

In SelectionDAGLegalize::ExpandNode, the implementation of the switch cases for ISD::STRICT_FP_ROUND and ISD::STRICT_FP_EXTEND does not respect strict FP mode, and therefore should be skipped if strict mode is enforced.   So you might want to change

  // This expansion does not honor the "strict" properties anyway,
  // so prefer falling back to the non-strict operation if legal.
  if (TLI.getStrictFPOperationAction(Node->getOpcode(),
                                     Node->getValueType(0))
      == TargetLowering::Legal)
    break;

to something like

  // This expansion does not honor the "strict" properties,
  // so we cannot use it if strict mode is enforced.
  if (DisableStrictNodeMutation || TLI.isStrictFPEnabled())
    break;
  // If strict mode is not enforced, and the non-strict operation
  // is legal, we might as well fall back to that.
  if (TLI.getStrictFPOperationAction(Node->getOpcode(),
                                     Node->getValueType(0))
      == TargetLowering::Legal)
    break;

The second place is this shortcut in VectorLegalizer::LegalizeOp

  // If we're asked to expand a strict vector floating-point operation,
  // by default we're going to simply unroll it.  That is usually the
  // best approach, except in the case where the resulting strict (scalar)
  // operations would themselves use the fallback mutation to non-strict.
  // In that specific case, just do the fallback on the vector op.
  if (Action == TargetLowering::Expand &&
      TLI.getStrictFPOperationAction(Node->getOpcode(),
                                     Node->getValueType(0))
      == TargetLowering::Legal) {
    EVT EltVT = Node->getValueType(0).getVectorElementType();
    if (TLI.getOperationAction(Node->getOpcode(), EltVT)
        == TargetLowering::Expand &&
        TLI.getStrictFPOperationAction(Node->getOpcode(), EltVT)
        == TargetLowering::Legal)
      Action = TargetLowering::Legal;
  }

This whole logic is only valid if we are allowed to fall back to non-strict expansion, so it should also be guarded by a !StrictFPEnabled check.

Finally, just a minor nit pick: it seems odd to always have to check both DisableStrictNodeMutation  and TLI.isStrictFPEnabled().  Can't we incorporate the command line override check into the TLI callback (e.g. by setting the default value of the flag depending on the command line variable)?


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

https://reviews.llvm.org/D70226