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

Mon Jun 26 23:26:00 PDT 2017

jbhateja marked 2 inline comments as done.
jbhateja added a comment.

Hi Eli,

Regarding your broadly two concerns raised above here are my responses :-

1/ We are not producing any illegal type post type legalization here.

Kindly have a look at following code snippet from function SelectionDAGLegalize::ExpandNode()

{

.......
 .......
case ISD::UMULO:
case ISD::SMULO: {

  EVT VT = Node->getValueType(0);
  EVT WideVT = EVT::getIntegerVT(*DAG.getContext(), VT.getSizeInBits() * 2);  

........
........
}

For UMULO and SMULO operantion since we need to test overflow hence during legalization (not type legalization) result type of the operation requested is WideVT which is twice the size of the result VT in this case it mean i128.

2/ On returning (i64, i64) in both the cases ie. when CallLoweringReturn is true or false. This is exactly what is being done NOW, earlier we were returning (i64,i64 BUILD_PAIR) when CallLoweringReturn was TRUE and i128 into a stack location when CallLoweringReturn was FALSE. NOW, instead of returing i128 into a stack location we return a pair of stack locations having VTs as (i64, i64).

Are you suggesting that breaking down of i128 held in the result stack location should be done outside the LowerCallTo function ?

That would mean defering the split decision from place where we exactly know whether the result is a stack location and type is illegal to a point where we have to put additional checks to test if type is legal or not and if its in a stack location. Also we are already past type legalization.

Inside LowerCallTo when CallLoweringReturn is FALSE (i.e. result is returned over
stack) we split ONLY IFF the result at the stack is not accomodable in the largest register, so this is safe. Let me know if you still have any concern.

Thanks.

================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8121
+
+        RetTys.clear();
+        RetTys.resize(NumValues);
----------------
efriedma wrote:
> 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 
Hi Eli,

Regarding your broadly two concerns raised above here are my responses :-