<html><head><meta http-equiv="Content-Type" content="text/html charset=windows-1252"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space;">OK, I can see that it works. It seems like you’re basically implementing the same idiom as the existing FoldingSet class (<a href="http://llvm.org/docs/ProgrammersManual.html#llvm-adt-foldingset-h">http://llvm.org/docs/ProgrammersManual.html#llvm-adt-foldingset-h</a>). Can you see if you could make use of that rather than rolling your own solution?<div><br></div><div>—Owen</div><div><br><div><div>On Oct 10, 2013, at 10:26 PM, Wan, Xiaofei <<a href="mailto:xiaofei.wan@intel.com">xiaofei.wan@intel.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-size: 12px; font-style: normal; font-variant: normal; font-weight: normal; letter-spacing: normal; line-height: normal; orphans: auto; text-align: start; text-indent: 0px; text-transform: none; white-space: normal; widows: auto; word-spacing: 0px; -webkit-text-stroke-width: 0px;">Owen:<br><br>Thanks for your comments.<span class="Apple-converted-space"> </span><br>Patch updated<span class="Apple-converted-space"> </span><a href="http://llvm-reviews.chandlerc.com/D1127">http://llvm-reviews.chandlerc.com/D1127</a><br><br>SDVTList SelectionDAG::getVTList(EVT VT1, EVT VT2) {<br> const EVT tmpVTs[2] = {VT1, VT2};<br> return getVTList(tmpVTs, 2);<br>}<br>How does the lifetime of this array work out? It looks like SDVTListInfo captures the pointer to the EVT array, which was stack allocated. After this method returns, you're going to have a dangling pointer in the SDVTListInfo.<br>[Xiaofei] Yes, tmpVTs is an array on the stack, but it is just used temporally to determine whether this SDVTList exist in the DenseMap, it won't be referenced anymore after that.<br><br>SDVTList SelectionDAG::getVTList(const EVT *VTs, unsigned NumVTs) {<br> SDVTListInfo Key(VTs, NumVTs);<br> if (VTListMap.count(Key))<br> return VTListMap[Key];<br><br> EVT *Array = Allocator.Allocate<EVT>(NumVTs);<br> std::copy(VTs, VTs + NumVTs, Array); -------------------------------------------------VTs is copied out to Array which will be stored in the DenseMap<br> SDVTList Result = makeVTList(Array, NumVTs);<br> Key.pVTs = Array; --------------------------------------------------VTs is not used any more.<br> VTListMap.insert(std::make_pair(Key, Result));<br><br> return Result;<br>}<br><br>Thanks<br>Wan Xiaofei<br><br>-----Original Message-----<br>From: Owen Anderson [<a href="mailto:resistor@mac.com">mailto:resistor@mac.com</a>]<span class="Apple-converted-space"> </span><br>Sent: Friday, October 11, 2013 1:30 AM<br>To: Wan, Xiaofei<br>Cc:<span class="Apple-converted-space"> </span><a href="mailto:llvm-commits@cs.uiuc.edu">llvm-commits@cs.uiuc.edu</a><span class="Apple-converted-space"> </span>LLVM<br>Subject: Re: [PATCH] [Review Request] Improve getVTList() in SelectionDAG<br><br>Some feedback:<br><br><blockquote type="cite">+ /// getEmptyKey() - A private constructor that returns an unknown<span class="Apple-converted-space"> </span><br>+ that is /// not equal to the tombstone key or SDVTListInfo().<br>+ static SDVTListInfo getEmptyKey() {<br>+ SDVTListInfo SDVTInfo;<br>+ SDVTInfo.NumVTs = 0;<br>+ return SDVTInfo;<br>+ }<br>+<br>+ /// getTombstoneKey() - A private constructor that returns an<span class="Apple-converted-space"> </span><br>+ unknown that /// is not equal to the empty key or SDVTListInfo().<br>+ static SDVTListInfo getTombstoneKey() {<br>+ SDVTListInfo SDVTInfo;<br>+ SDVTInfo.NumVTs = 0;<br>+ return SDVTInfo;<br>+ }<br></blockquote><br>I don't think this is a valid implementation of the DenseMapInfo callbacks. getEmptyKey() and getTombstoneKey() have to return values that don't compare equal to each other.<br><br><blockquote type="cite">+ const EVT pVTs[2] = {VT1, VT2};<br>+ return getVTList(pVTs, 2);<br></blockquote><br>How does the lifetime of this array work out? It looks like SDVTListInfo captures the pointer to the EVT array, which was stack allocated. After this method returns, you're going to have a dangling pointer in the SDVTListInfo.<br><br>-Owen<br><br><span><D1127.5.patch></span></div></blockquote></div><br></div></body></html>