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

Jatin Bhateja via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Sun Jun 25 19:42:47 PDT 2017


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

ping [eli or others]@reviewers



================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3564
+      BottomHalf = Ret.getOperand(0);
+      TopHalf = Ret.getOperand(1);
     }
----------------
vadimcn wrote:
> vadimcn wrote:
> > efriedma wrote:
> > > jbhateja wrote:
> > > > efriedma wrote:
> > > > > jbhateja wrote:
> > > > > > vadimcn wrote:
> > > > > > > So this assumes that the type of node node returned by ExpandLibCall is known and not going to change?   Seems a bit dangerous to me...
> > > > > > Please consider the fact that node being returned from LowerToCall is of type MERGE_VALUES which is a collection of nodes, EXTRACT_ELEMENT is used to extract part value constructed using BUILD_PAIR. BUILD_PAIR makes a larger value type. In this case since call lowering is happening during node legalization (called post type legalization) so returning a collection of stack locations which cumulatively stores the larger result looks fine. We just need to access the operands of MERGE_VALUES to get the Bottom and Top halves.
> > > > > Yes, definitely wrong: consider the case where the double-width type is legal.
> > > > Hi Eli, This case is under condition when CanLowerReturn is false, i.e. type is illegal in TargetLowring::LowerCallTo. Also in SelectionDAGLegalize::ExpandNode RTLLIB call is  considered for UMULO operation only when return type is illegal. 
> > > > Change has been run through CodeGen regressions which is all clear.
> > > > 
> > > > 
> > > Oh, right, sorry, missed the isTypeLegal check. I guess if UMULO result type is legal, and a type twice that width isn't, it should always get split into precisely two nodes.
> > > 
> > > Please add an assertion that Ret is an ISD::MERGE_VALUES with two operands (assuming that's what it's supposed to be?).
> > Jatin, my issue with this code is that it seems to just assume that Ret.getOpcode() == ISD::MERGE_VALUES.   Which may well be true now, but this code is pretty far away from where the Ret node gets constructed.   What if tomorrow somebody modifies ExpandLibCall and it starts returning something else?
> > 
> > I think that you should assert the node type here before using its operands.  (Unless, of course, MERGE_VALUES is the only node type that makes sense here.  But even then, this fact merits at least being stated in a comment)
> > 
> I am wondering if this handling of EXTRACT_ELEMENT(MERGE_VALUES) could be pushed to SelectionDAG::getNode(), where other const folding already takes place?
Hi Vadimcn,

Please look at following exceprts from code ( SelectionDAG::getNode() : SelectionDAG.cpp)

1/ EXTRACT_ELEMENT BUILD_PAIR. instruction folding from SelectionDAG::getNode() ->

// EXTRACT_ELEMENT of BUILD_PAIR is often formed while legalize is expanding
// 64-bit integers into 32-bit parts.  Instead of building the extract of
// the BUILD_PAIR, only to have legalize rip it apart, just do it now.
if (N1.getOpcode() == ISD::BUILD_PAIR)
  return N1.getOperand(N2C->getZExtValue());

2/ Handling in SelectionDAG::getNode() for single argument MERGE_VALUE

switch (Opcode) {
case ISD::TokenFactor:
case ISD::MERGE_VALUES:
case ISD::CONCAT_VECTORS:
  return Operand;         // Factor, merge or concat of one node?  No need.

So an ExpandLibCall result if comprise of build pair it will be naked i.e. not contained in MERGE_VALUE node and its operands can be accessed directly which is what insturction folding in getNode doing.

Fix done for this bug is to break the stack locations into multiple results and sending it back after packing them in a MERGE_VALUE node.

This is a special case because larger unsupproted type (i128) is not the actual return value type of llvm.umulo otherwise DAG type legalization would have broken it down to legal 
types earlier itself.

Thanks


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


https://reviews.llvm.org/D34240





More information about the llvm-commits mailing list