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

Vadim Chugunov via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Thu Jun 22 00:14:01 PDT 2017


vadimcn added inline comments.


================
Comment at: lib/CodeGen/SelectionDAG/LegalizeDAG.cpp:3564
+      BottomHalf = Ret.getOperand(0);
+      TopHalf = Ret.getOperand(1);
     }
----------------
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?


https://reviews.llvm.org/D34240





More information about the llvm-commits mailing list