Page MenuHomePhabricator

[SelectionDAGBuilder] Remember to copy the FMF flags in visitBinary().
AbandonedPublic

Authored by jonpa on Sep 4 2020, 1:44 AM.

Details

Reviewers
spatel
uweigand
Summary

This is a follow-up patch for the bugfix of https://reviews.llvm.org/D86871.

It fixes the XFAILING SystemZ/fp-mul-14.ll test, and also improves two others.

(D86871 is still pending, and this will be committed afterwards to fix its XFAILing test)

Diff Detail

Event Timeline

jonpa created this revision.Sep 4 2020, 1:44 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 4 2020, 1:44 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
jonpa requested review of this revision.Sep 4 2020, 1:44 AM

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

jonpa added a comment.Sep 4 2020, 7:31 AM

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

The other thing that was removed by that commit was the propagation of the NoNaNs flag from the experimental_vector_reduce_fmax (/fmin) intrinsic to the replacing VECREDUCE_FMAX (/FMIN) node, but I am not sure why...

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

The other thing that was removed by that commit was the propagation of the NoNaNs flag from the experimental_vector_reduce_fmax (/fmin) intrinsic to the replacing VECREDUCE_FMAX (/FMIN) node, but I am not sure why...

Well, the whole approach of D46854 was to remove setting of the FMF in DAG.getNode() everywhere, and instead set the flags via setFlags() after the fact. The point was to be that setting the flags in one place reduces code duplication. However, we're now having the discussion that using setFlags() after the fact may be incorrect in the presence of memoized nodes, and instead we should be always setting flags directly in getNode() -- this basically argues for exactly the opposite direction of what that commit did.

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

The other thing that was removed by that commit was the propagation of the NoNaNs flag from the experimental_vector_reduce_fmax (/fmin) intrinsic to the replacing VECREDUCE_FMAX (/FMIN) node, but I am not sure why...

Well, the whole approach of D46854 was to remove setting of the FMF in DAG.getNode() everywhere, and instead set the flags via setFlags() after the fact. The point was to be that setting the flags in one place reduces code duplication. However, we're now having the discussion that using setFlags() after the fact may be incorrect in the presence of memoized nodes, and instead we should be always setting flags directly in getNode() -- this basically argues for exactly the opposite direction of what that commit did.

The after the fact handling also caused issue with constrained fp intrinsics which is why they were already excluded. It also had an issue with the vector reduction flag we used to have until I removed it in https://reviews.llvm.org/D76649.

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

The other thing that was removed by that commit was the propagation of the NoNaNs flag from the experimental_vector_reduce_fmax (/fmin) intrinsic to the replacing VECREDUCE_FMAX (/FMIN) node, but I am not sure why...

I think this makes sense to me, but I want to point out that this partially reverts the commit https://reviews.llvm.org/D46854. I guess there may be other parts of this commit that should be reverted?

The other thing that was removed by that commit was the propagation of the NoNaNs flag from the experimental_vector_reduce_fmax (/fmin) intrinsic to the replacing VECREDUCE_FMAX (/FMIN) node, but I am not sure why...

Well, the whole approach of D46854 was to remove setting of the FMF in DAG.getNode() everywhere, and instead set the flags via setFlags() after the fact. The point was to be that setting the flags in one place reduces code duplication. However, we're now having the discussion that using setFlags() after the fact may be incorrect in the presence of memoized nodes, and instead we should be always setting flags directly in getNode() -- this basically argues for exactly the opposite direction of what that commit did.

Right - we were trying to make a single point for transferring FMF instead of having every visitXXX() fend for itself. But if there's no way to do it safely/correctly, let's reverse course. At this point, I'm not sure if there is still some problem with always adding flags at getNode() time. Ie, we added the 'Defined' state bit to solve some other flags transfer problem, but is that still needed?

jonpa abandoned this revision.Sep 7 2020, 1:30 AM

Not needed anymore thanks to D87200.