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

[Eli Friedman via Phabricator via llvm-commits]

efriedma added a comment.

I apologize, my review last night wasn't very insightful.  Hopefully better comments today.

---

test/CodeGen/ARM/umulo-32.ll and test/CodeGen/SPARC/64cond.ll are tests for other targets which use this codepath.  Please verify that you aren't changing the generated code for those testcases.  (I guess wasm in particular breaks it can't return an i128 in registers?)



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


================
Comment at: lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:8121
+
+        RetTys.clear();
+        RetTys.resize(NumValues);
----------------
I'm a little suspicious about the way you're messing with RetTys here; the contents shouldn't depend on  CanLowerReturn.


https://reviews.llvm.org/D34240