<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On 15 June 2015 at 16:01, Sanjay Patel <span dir="ltr"><<a href="mailto:spatel@rotateright.com" target="_blank">spatel@rotateright.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">In <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10403-23188316&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=sfmI3K06lugS09A3HoKs7UKw5kIj6DYSMaeTdJSq_2M&s=1HNZ_zVmFhjlidkJ82WQ8CXLyJQZ2lBsMCtWd1T85DU&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10403#188316</a>, @nlewycky wrote:<br>
<br>
> Are you sure you don't need to re-enable any tests with this commit? I'm concerned that there's no test change.<br>
<br>
<br>
</span>There should be no visible changes from adding the FP flags because we're not detecting them for optimizations anywhere in the DAG. That's why I naively marked the previous patches as 'NFC'. :)<br>
<br>
The existing tests for Exact / NSW / NUW verify that we're still able to do optimizations based on those bits.<br>
<br>
My first proposed patch for optimization using the FP flags ( <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9708&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=sfmI3K06lugS09A3HoKs7UKw5kIj6DYSMaeTdJSq_2M&s=wXSHAQ_K96h5HwuksOUtSCij7WEamDEoKZ6S3ARGcpo&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D9708</a> ) just happened to be the same spot where you discovered a bug ( <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D9893&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=sfmI3K06lugS09A3HoKs7UKw5kIj6DYSMaeTdJSq_2M&s=UvT_fJ2XjXKNA2nw8bdHD19InkrNUsOeMNrOjAt_Jp4&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D9893</a> )!<br></blockquote><div><br></div><div>Ok.</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><span class="">================<br>
Comment at: lib/CodeGen/SelectionDAG/SelectionDAG.cpp:410-412<br>
@@ +409,5 @@<br>
+  unsigned RawFlags = Flags->getRawFlags();<br>
+  // If no flags are set, do not alter the ID. This saves time and allows<br>
+  // a gradual increase in API usage of the optional optimization flags.<br>
+  if (RawFlags != 0)<br>
+    ID.AddInteger(RawFlags);<br>
----------------<br>
</span><span class="">nlewycky wrote:<br>
> Are you saying that the FoldingSetNodeID is computed elsewhere too, without using this function? And that we don't want to call AddInteger one more time because we don't want to make it different from that other place of computation?<br>
><br>
</span>Yep, that's the idea.</blockquote><div><br></div><div>The comment should say that outright; as written it reads that it's a performance issue and an API rollout issue, but leaves out the 'we must not add this integer to the set for correctness' issue.</div><div><br></div><div>LGTM</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> The new 'getNode' API has a nullptr default for the flags:<br>
SDValue getNode(unsigned Opcode, SDLoc DL, EVT VT, SDValue N1, SDValue N2,<br>
                  const SDNodeFlags *Flags = nullptr);<br>
<br>
So we're not updating every use of that function to explicitly include a Flags ptr. If some path into here is using Flags but none are set, then we want to match the folding set node that would be created by those un-Flag'ified uses of getNode().<br>
<div class="HOEnZb"><div class="h5"><br>
<a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_D10403&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=sfmI3K06lugS09A3HoKs7UKw5kIj6DYSMaeTdJSq_2M&s=74sthUAOhT7tKXNILMSwD-09aTl4-7blrKsl27oH29s&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/D10403</a><br>
<br>
EMAIL PREFERENCES<br>
  <a href="https://urldefense.proofpoint.com/v2/url?u=http-3A__reviews.llvm.org_settings_panel_emailpreferences_&d=AwMFaQ&c=8hUWFZcy2Z-Za5rBPlktOQ&r=mQ4LZ2PUj9hpadE3cDHZnIdEwhEBrbAstXeMaFoB9tg&m=sfmI3K06lugS09A3HoKs7UKw5kIj6DYSMaeTdJSq_2M&s=-WuD0hhAJUBzJAC4T6F0F7QCxDI2NwLGMHxblMY6jQ0&e=" rel="noreferrer" target="_blank">http://reviews.llvm.org/settings/panel/emailpreferences/</a><br>
<br>
<br>
</div></div></blockquote></div><br></div></div>