[PATCH] D34240: [WebAssembly] Expansion of llvm.umul.overflow with i64 type operands.

[Jatin Bhateja via Phabricator via llvm-commits]

jbhateja marked an inline comment as done.
jbhateja added a comment.

In https://reviews.llvm.org/D34240#792459, @efriedma wrote:

> > earlier we were returning (i64,i64 BUILD_PAIR) when CallLoweringReturn
>
> Despite the name, BUILD_PAIR does not return a pair.  It returns an i128.  It's essentially just a shortcut for `((zext Op1 to i128) << 64) | (zext Op0 to i128))` (see SelectionDAGLegalize::ExpandNode).  This is a node with an illegal result type, and we should not be creating it.
>
>  ---
>
> The changes you're making to LowerCallTo in the case where CanLowerReturn is false look good.
>
> The part I'm not happy with is that LowerCallTo behaves inconsistently with your patch.  In the case where CanLowerReturn is true, we should split the returned value in the same way you're splitting it when CanLowerReturn is false.


As per your above comment ->  "The particular code you're looking at managed to slip past at some point; we didn't fix it properly, and instead depend on an ugly hack to hide the illegal node from LegalizeDAG. As you discovered, the trick only works if the node is a BUILD_PAIR; we don't have code here to split loads."

LowerCallTo does not behave inconsitantly with my patch, thats the way it was working always.  I just split the illegal type stack location into multiple legal type stack locations..

Just to bring it to your notice again this is a special case as WideVT (duble to type of original result) is being defined post type legalization which is why
we need to handle this illegal return type spilliting in LowerCallTo so that it returns a legal result values to it caller.

However, I definately agree that in long term this entire piece of code should be re-written so that LowerCallTo returns only one consistant type of node, but considering that argument to call being
lowered can also be non-integral types like a structure pointer , hence not sure/convinced that returing a result of node type BUILD_PAIR in all cases would be feasible, also LowerCallTo is being called from several places during type legalization also where SplitInteger is being explicitely called on return value to extract constituent elements.  So making radical changes in LowerCallTo by returing only 
one kind of node which eventually should be MERGE_VALUE might need good deal to work and testing.

As of now this is not happening but the patch is not breaking anything and it covering a case which was missed about spliting the illlegal type result.

Thanks


https://reviews.llvm.org/D34240