[PATCH] D86871: [SelectionDAG] Let NSW/NUW flags be cleared by default in call to getNode().

[Amara Emerson via Phabricator via llvm-commits]

aemerson added a comment.

In D86871#2254760 <https://reviews.llvm.org/D86871#2254760>, @jonpa wrote:

> Patch updated per review.
>
>> I'm questioning why we should have that "isDefined" logic in the first place. It seems to me that any node with "undefined" flags can only be used correctly if any setting of the flags would be semantically correct -- but then we may just as well use the all-zero setting of the flags anyway.
>
> I changed te uses in the AMDGPU backend so that it is now obvious that the only reason for having isDefined() is to allow SelectionDAGBuilder to set the flags at a later point than when creating the nodes.
>
> To get rid of it I think this is needed:
>
> - Refactor SelectionDAGBuilder to call setValue(DAG.getNode()) with the correct SDNodeFlags directly.
> - SelectionDAGBuilder::Visit(const Instruction) call to setFlags() can be removed.
> - the isDefined() and AnyDefined member of SDNodeFlags can then be removed.
>
>> I still believe that even with that change most (all?) uses of setFlags are dangerous as they could introduce a flag into a shared node that may not be appropriate for all existing uses. But maybe that can then be a separate discussion elsewhere.
>
> I may be missing something, but as long as the memoization does a true intersection of the flags per this patch, there should not necessarily be anything broken, or?
>
> I think it would be possible to make things clearer:
>
> - When a new equivalent node takes over flags from a node it is replacing (for instance during widening), it should pass the flags to getNode() directly or if there are many calls to getNode() maybe call a function like transferFlags(OrigNode, NewNode) at the end of the function.
> - When a new node is created and the values of the flags are given by the context, the flags should be passed directly to getNode().
> - When a single flag needs to be set on an existing node, why not do it directly instead of calling getFlags(), set...(), setFlags() (like SelectionDAGISel currently does for setNoFPExcept).
> - ...?
>
> I added a TODO comment referencing this.
>
>> I agree removing the isDefined() check within intersectWith() is a good idea, and if we can do it without regression, we should do so. (And that will certainly fix the original problem Jonas was seeing.
>
> Yes, all the tests are passing :-) With fast-fp, there are 2 files changing on SPEC with a total of just 8 less fused ops (without fast fp, there is no change). This is due to SelectionDAGBuilder now clearing the flags of the fmul and fadd nodes so DAGCombiner will not produce the fma. I would hope this is temporarily acceptable, or?
>
> @aemerson: It seems you introduced the isDefined() with d28f0cd4 - are you OK with this change, and what are your comments now?

I don't have any strong feelings about this. I agree with Sanjay that correctness is more important but it would be good to have some tests that show what we're now failing to optimize.


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

https://reviews.llvm.org/D86871