Page MenuHomePhabricator

Add an option to disable strict float node mutating to an normal float node
ClosedPublic

Authored by LiuChen3 on Thu, Nov 14, 2:39 AM.

Details

Summary

This patch add an option 'disable-strictnode-mutation' to prevent strict node mutating to an normal node.
So we can make sure that the patch which sets strict-node as legal works correctly.

Diff Detail

Event Timeline

LiuChen3 created this revision.Thu, Nov 14, 2:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptThu, Nov 14, 2:39 AM

There are some tests fail on SystemZ and I simply transferred
thoses testcases which operations are not legal to a new file.

I don't believe those tests ought to fail. They're all cases where a FP operation is implemented via a library call. Those should be fine for strict FP semantics, so we should get this even with -disable-strictnode-mutation.

I think the option should only disable the call to mutateStrictFPToFP in SelectionDAGISel. The calls in SelectionDAGLegalize::ExpandFPLibCall and SelectionDAGLegalize::ExpandArgFPLibCall are fine IMO.

However, I think the option should in addition disable the special handling of strict FP operations in SelectionDAGLegalize::ExpandNode. Those are only valid because the code expects the mutateStrictFPToFP call to happen in SelectionDAGISel. If this won't happen, the special handling during Expand shouldn't happen either.

Also, I think it would be nice if a target could default to having -disable-strictnode-mutation always on. We'd want to do that on SystemZ. (Then you wouldn't have to change all the test cases either.)

LiuChen3 updated this revision to Diff 229459.Fri, Nov 15, 12:44 AM
LiuChen3 edited the summary of this revision. (Show Details)

This patch add an variable EnableStrictNode to the TargetLoweringBase to determine whether the target already supports the strict float operation.
Modify the patch @uweigand's suggestion.

@uweigand Thanks for your review. I have added information to see if the target supports strict float, so systemZ can disable strictnode mutation as default.

pengfei added inline comments.Fri, Nov 15, 1:08 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
3716

Is it better to use !(TLI.isStrictFPEnabled() || DisableStrictNodeMutation) ?

llvm/lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1160

Same as above.

llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
639

Extra blank.

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

LiuChen3 updated this revision to Diff 229799.Mon, Nov 18, 4:04 AM

Binding the value of DisableStrictNodeMutation to flag IsStrictFPEnabled, updating as comments.

LiuChen3 marked an inline comment as done.Mon, Nov 18, 4:11 AM
LiuChen3 added inline comments.
llvm/lib/Target/SystemZ/SystemZISelLowering.cpp
639

Thanks for your review, there is no blank here original.

I'm not sure if there's a possibility: some normal float operations of a backend is not Expand,
but they may think expanding strict-float operation can meet their requirements.
Although I haven't found a case yet.

uweigand added inline comments.Mon, Nov 18, 4:33 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2658

I'm not sure this is always correct, I think it might be possible that a target might want to select Expand for a strict operation even if they use e.g. Custom for the non-strict version (obviously, that would have to be an operation where common code implements an Expand algorithm that respects the constrained FP semantics).

More importantly, even if you do this, you still need to add the checks in STRICT_FP_ROUND and STRICT_FP_EXTEND I mentioned in my earlier comment: note that in those cases, even if the target uses Expand for both the strict and non-strict operation, the code below still cannot be used if isStrictFPEnabled is true (since it does not respect constrained FP semantics).

LiuChen3 marked an inline comment as done.Mon, Nov 18, 5:08 AM
LiuChen3 added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2658

Thanks.
I think I misunderstood what you meant before. You actually mean is if the backend has supported strict float, it can never expand STRICT_FP_ROUND and STRICT_FP_EXTEND operations. We don't expand it not because we setOperationAction wrong or something else, because it isn't 'strict float' at all.
I'll only add judgment based on the previous patch and delete this.

pengfei added a subscriber: kpn.Mon, Nov 18, 6:56 AM
pengfei added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2658

In my opinion, the behavior currently is reasonable. I don't think there's a way in common code can handle an expand strict node if its non-strict node is custom. Otherwise, its non-strict node isn't necessarily to be custom.
For STRICT_FP_ROUND and STRICT_FP_EXTEND, I reviewed the discussion between you and @kpn in D65226. I think it's equal to your change if the action of the target's non-strict nodes is legal. And for target that isStrictFPEnabled, the action of the strict nodes can not be set to expand if the expansion does not respect constrained FP semantics.

In my opinion, the behavior currently is reasonable. I don't think there's a way in common code can handle an expand strict node if its non-strict node is custom. Otherwise, its non-strict node isn't necessarily to be custom.

What I was thinking about is: there are some operations where we can have an Expand implementation that correctly respects strict semantics, typically by mapping on top of (other) strict operations. E.g. an implementation of STRICT_UINT_TO_FP in terms of STRICT_SINT_TO_FP (see discussion in D69275). In those cases, the target should be able to select that expansion, even if it has a UINT_TO_FP custom handler (for whatever reason, maybe for some optimizations that aren't possible in the strict case). There's no particular reason to disallow this.

And for target that isStrictFPEnabled, the action of the strict nodes can not be set to expand if the expansion does not respect constrained FP semantics.

But it may still be possible to respect strict semantics by expanding to a libcall -- and that is what should happen in those cases, I think.

In summary, there are four potential cases how Expand of a STRICT node could be implemented:

  1. A custom expansion sequence that respects constrained semantics
  2. Expansion to libcall (where the library is assumed to respect constrained semantics)
  3. A custom expansion sequence that does not respect constrained semantics
  4. "Fake" expansion to the non-strict node

If isStrictFPEnabled is true, then cases 3) and 4) above are forbidden, but cases 1) and 2) are still OK.

My main point is that the difference between 1) and 3) has to be determined on a case-by-case basis by inspecting the particular expansion sequence, and therefore this should be checked in-line in each expansion sequence. This is why I'd prefer to have each affected custom expansion sequence implementation directly the flag check whether or not strict semantics must be enforced or not; I don't believe this can be a single check just at the top of ExpandNode.

My main point is that the difference between 1) and 3) has to be determined on a case-by-case basis by inspecting the particular expansion sequence, and therefore this should be checked in-line in each expansion sequence. This is why I'd prefer to have each affected custom expansion sequence implementation directly the flag check whether or not strict semantics must be enforced or not; I don't believe this can be a single check just at the top of ExpandNode.

Thanks for the explanation! It very helpful for us to understand the workflow of strict semantics.

LiuChen3 updated this revision to Diff 229958.Mon, Nov 18, 6:37 PM

Update as comments.

LiuChen3 updated this revision to Diff 229962.Mon, Nov 18, 6:45 PM

Fix format problem.

Thanks! Just one minor nit about the comment inline, otherwise this patch LGTM.

llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2808–2813

This comment is now duplicated; it would be better to change the comment above to something along the lines of what I suggested earlier, e.g.

// This expansion does not honor the "strict" properties,
// so we cannot use it if strict mode is enforced.
2834–2839

See above.

I'm strongly considering making mutation a true operation action like Expand, Legal, Custom, etc. So we can distinquish Expand from "my target doesn't support strict FP yet". The checks for "legal" on the non-strict nodes to guess what we should do, don't work for X86 where we have a lot of Custom handling. The strict fp operations would default to this new operation action instead of Expand.

LiuChen3 updated this revision to Diff 230173.Tue, Nov 19, 4:25 PM

Modify comment

@uweigand Thanks for your help.

I'm strongly considering making mutation a true operation action like Expand, Legal, Custom, etc. So we can distinquish Expand from "my target doesn't support strict FP yet". The checks for "legal" on the non-strict nodes to guess what we should do, don't work for X86 where we have a lot of Custom handling. The strict fp operations would default to this new operation action instead of Expand.

Hmm. I'd actually consider this a step in the wrong direction, since the mutation operation is really wrong, it doesn't actually respect strict fp semantics. So I'd rather have it go away completely as soon as possible -- once we have enough target coverage (e.g. X86, Arm, Power, SystemZ?). That's why I like the approach in this patch: all the mutation-related stuff is now behind the isStrictFPEnabledCheck, and soon as we decide target support is sufficient, we just remove all that code. (For targets that still don't support strict FP natively, the intrinsics would then generally map to libcalls.)

uweigand added inline comments.Wed, Nov 20, 4:25 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2814

Sorry, this is still not quite what I expected: now you've removed the second comment, which was actually correct (and necessary) ...

My point is that the two "if" statements implement two very different things, the first is a correctness issue, the second is just a performance optimization. So we really ought to have two different comments explaining the two different purposes of those if statements, as I had in my original suggestion.

In your first patch that I commented upon earlier, you had two comments, but both were talking about the performance optimization -- this is wrong for the first if, which is all about correctness. Now you fixed the comment before the first if to talk about correctness, but you removed the second comment completely, which gives the impression that the second if is also about correctness, which it is not ...

This comment was removed by LiuChen3.
LiuChen3 updated this revision to Diff 230253.Wed, Nov 20, 7:03 AM

update the comments

LiuChen3 marked an inline comment as done.Wed, Nov 20, 7:20 AM
LiuChen3 added inline comments.
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2814

Thanks for your explanation. But why this is a performance optimization? I thought this conversion was just to allow the backend to make the correct instruction selection without supporting strict-float. The performance optimization means by the promotion of the legal instruction compared to the converting to statck operation?
Or I misunderstand something?

uweigand added inline comments.Wed, Nov 20, 8:20 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2814

OK, so when we get here, the back-end has asked common code to "Expand" the STRICT_FP_ROUND operation. Common code has three options to do so:

  1. Emit a libcall
  2. Replace it with a FP_ROUND -- only possible if FP_ROUND is "Legal"
  3. Replace it with a stack operation (truncating store followed by load)

If we must enforce strict FP semantics, then only option 1) is allowed, since both options 2) and 3) do not respect that semantics. That is the correctness property that is enforced by the first "if".

Now, if we do not have to enfore strict FP semantics, then either option 1), 2) or 3) would be allowed. So in case, we make the decision on the relative efficiency of those options, where we'd usually have 2) the fastest, followed by 3), and then 1) as the slowest. Since 2) is not always possible, we'd choose 2) when it is available, and 3) otherwise. This is what the second "if" achieves.

Does this make it clearer? If you find some other wording for those comments that convey that explanation in a better way, feel free to update them :-)

craig.topper added inline comments.Wed, Nov 20, 8:59 AM
llvm/lib/CodeGen/SelectionDAG/LegalizeDAG.cpp
2814

X86 has FP_ROUND marked Custom, but most type combinations are Legal. I had to mark STRICT_FP_ROUND as Custom to get it past this code. But now I can’t get it past the mutation code in SelectionDAGIsel because it’s not “Legal”.

Scalar FADD on X86 is also marked Custom but most cases go through unmodified. STRICT_FADD is marked Expand currently. And only doesn’t get turned into a lib call because I don’t think there is STRICT_FADD libcall support yet. But that needs to be added to support strict ops on f128 for X86-64. The moment that happens then every other target that hasn’t implemented strict fp yet will generate a libcall for STRICT_FADD.

X86 has FP_ROUND marked Custom, but most type combinations are Legal. I had to mark STRICT_FP_ROUND as Custom to get it past this code. But now I can’t get it past the mutation code in SelectionDAGIsel because it’s not “Legal”.

Ah, so you mark STRICT_FP_ROUND Custom, but in the custom expander still leave it as STRICT_FP_ROUND, expecting it to be matched? I see. I believe this code in SelectionDAGISel:

if (Node->isStrictFPOpcode() &&
    (TLI->getOperationAction(Node->getOpcode(), Node->getValueType(0))
     != TargetLowering::Legal))

should really be:

if (Node->isStrictFPOpcode() &&
    (TLI->getOperationAction(Node->getOpcode(), Node->getValueType(0))
     == TargetLowering::Expand))

That should fix your problem. (Or else, once we get this patch committed, you could also set isStrictFPEnabled for your target, and the problem would also be gone.)

Scalar FADD on X86 is also marked Custom but most cases go through unmodified. STRICT_FADD is marked Expand currently. And only doesn’t get turned into a lib call because I don’t think there is STRICT_FADD libcall support yet. But that needs to be added to support strict ops on f128 for X86-64. The moment that happens then every other target that hasn’t implemented strict fp yet will generate a libcall for STRICT_FADD.

Right. But as I said, I personally would prefer this behavior: at least the compiler doesn't silently ignore strict semantics that it promised to implement ...

LiuChen3 updated this revision to Diff 230365.Wed, Nov 20, 8:17 PM

update comments and fix a bug

@uweigand Thank you, now I get clear of the logic there. I add some of your comment to the original comments in the code, I think it may helps us understand this code better. Or we still keep the previous comments?

Thanks again! As far as I'm concerned, this now looks good and I'd like to see it go in -- @craig.topper: given your comments above, do you still have any objections?

I'm fine with this. I changed the code in SelectionDAGIsel yesterday to use Expand. So this will need to be rebased.

uweigand accepted this revision.Thu, Nov 21, 10:45 AM

OK, great. LGTM.

This revision is now accepted and ready to land.Thu, Nov 21, 10:45 AM

Thanks for all of your help.

This revision was automatically updated to reflect the committed changes.