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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8121
+
+        RetTys.clear();
+        RetTys.resize(NumValues);
----------------
jbhateja wrote:
> efriedma wrote:
> > efriedma wrote:
> > > I'm a little suspicious about the way you're messing with RetTys here; the contents shouldn't depend on  CanLowerReturn.
> > Okay, so this is doing what I thought it's doing.
> > 
> > Making the type of the node returned by TargetLowering::LowerCallTo different based on the target's calling convention is not acceptable.  Both sides of the "if (!CanLowerReturn)" need to do essentially the same thing.  (Basically, you need to skip calling getCopyFromParts in the CanLowerReturn case.)
> > 
> > The end result still isn't really quite right; we aren't passing the right argument types to the calling convention code.  But hopefully that problem won't bite anyone.
> Hi Eli, Types of node being returned could be different in IF and ELSE part of CanLoweReturn before this fix also. We can have BUILD_PAIR, or MERGE_VALUE both are possible based on number parts being returned.
> 
> Fix just splits larger integer result present at a stack locations to multiple smaller stack location such that the parts value types are legal as per target.
> 
> Also IsPostLegalization flag in Calling convention is specifying whether call lowering was done in Type Legalization of Selection DAG or during Node Legalization.
> 
> During Type Legalization if a larger illegal result is returned from LowerCallTo function then it is further broken down to legal type as type legalization is an iterative loop which messages the type till they are legal.
> 
> Thanks
After type legalization, we are not supposed to produce nodes with illegal types.  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.

If we're going to fix this properly, we should not be creating nodes with illegal types in call lowering after type legalization, whether or not CanLowerReturn is true.

The part I care about is that the VT of the returned node should be consistent.  If we're generating a call after legalization, and the return type of that call is an 128-bit integer, we should always return a node whose VT is the pair "(i64, i64)".  If we get it right, the details of call lowering are transparent; otherwise, we just have to hope we get lucky with the lowering of 


https://reviews.llvm.org/D34240