[PATCH] D35267: Pass Divergence Analysis data to selection DAG to drive divergence dependent instruction selection

[Alexander via Phabricator via llvm-commits]

alex-t added a comment.

In https://reviews.llvm.org/D35267#966245, @efriedma wrote:

> In https://reviews.llvm.org/D35267#962949, @alex-t wrote:
>
> > > This is only true if your original computation is correct, and if DAGCombine/Legalization doesn't create any nodes which are naturally divergent.  Neither of those are safe assumptions, I think.  DAGCombine and legalization will transform loads and stores, which could end up creating a naturally divergent node.
> >
> > So, my question is: could you imagine even theoretical sensible transformation that convert the graph in such a way that uniform node will get divergent income?
>
>
> No, but that isn't the point.  The problem is that you could replace a naturally divergent node with an equivalent naturally divergent node, but the new node doesn't have the divergent bit set (since the bit only gets set in DAGCombine for nodes with divergent operands, and naturally divergent nodes might not have divergent operands).  Thinking about it a bit more, I guess regular load/store operations are a bad example; if a load produced multiple values given a uniform address, it would be a data race.  But I think atomic memory operations could run into this issue?  (Consider, for example, the code in DAGTypeLegalizer::PromoteIntRes_Atomic1.)
>
> In https://reviews.llvm.org/D35267#962949, @alex-t wrote:
>
> > > And some divergent nodes will never be passed to SelectionDAGBuilder::setValue when you build the DAG, due to the way SelectionDAGBuilder handles values with illegal types.  But I'm not sure that's a complete list of the issues with the current version, and there's no practical way to check without a verifier.
> >
> > Even if it creates new DAG pattern it returns it's root that (because of CreateOperands) has correct divergence that will be passed to setValue. Or I did not understand what you meant?
>
>
> That's not what I meant.
>
> Say you have a call to a divergent function which returns an i64, but i64 isn't legal on your target (so the function effectively returns two values of type i32).  We create the call, a couple CopyFromReg nodes, and then a MERGE_VALUES to merge the value.  Then you set the MERGE_VALUES to be divergent... but that isn't really helpful: legalization for MERGE_VALUES erases the node, so the "divergent" bit goes away.


The diff uploaded is a draft just to check - does it look like what you meant? In fact there are some issues to resolve:

1. The content of the target specific "isSDNodeSourceOfDivergence" procedure depend on the stage of the DAG lowering where it is called. The most reasonable place is just before the selection after all combining/legalizing are done. In this case all the intrinsics are already expanded and turned to the CopyFromReg or similar elementary operations. So it is unclear if it reasonable to have the code handling this intrinsics.
2. All the divergence flags propagation in the "ReplaceAllUsesWith" are useless and should be removed.
3. This solution is not in fact verification because the flags computed on single block in general don't match those passed from the IR because of the control dependencies. This is just yet another part of analysis to augment the information.


https://reviews.llvm.org/D35267