<div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Tue, Apr 20, 2021 at 6:48 PM Philip Reames <<a href="mailto:listmail@philipreames.com">listmail@philipreames.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
<div>
<p><br>
</p>
<div>On 4/20/21 3:32 PM, Eric Christopher
wrote:<br>
</div>
<blockquote type="cite">
<div dir="ltr">
<div dir="ltr"><br>
</div>
<br>
<div class="gmail_quote">
<div dir="ltr" class="gmail_attr">On Tue, Apr 20, 2021 at 6:28
PM Philip Reames via Phabricator <<a href="mailto:reviews@reviews.llvm.org" target="_blank">reviews@reviews.llvm.org</a>>
wrote:<br>
</div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">reames added a comment.<br>
<br>
In D99976#2703175 <<a href="https://reviews.llvm.org/D99976#2703175" rel="noreferrer" target="_blank">https://reviews.llvm.org/D99976#2703175</a>>,
@echristo wrote:<br>
<br>
> I guess my question is "why? what does this make
easier/harder/etc?"<br>
<br>
A couple of pieces here.<br>
<br>
1. We no longer have the chance of a nasty false negative
bug. Before, we could fail to handle some intrinsics
because they would not match IntrinsicInst.<br>
2. As listed in the potential cleanups in the commit, we
have multiple places in code which handled intrinsics
generically, but then had to add extra casing for the
invokable ones. We'll be able to clean that up.<br>
3. For intrinsics which really are calls - which,
admittedly, is many - this does complicate the pattern
matching as you have to both match the intrinsicinst, and
then cast to CallInst for interfaces which expect calls. (A
quick survey shows many of those interfaces could reasonably
take CallBase, but not all.) We could explore having two
base classes - one for all intrinsics and one for call only
intrinsics - not quite sure if the multiple inheritance
works out there. It didn't seem worthwhile to justify
exploration to me, if you disagree, let me know.<br>
<br>
</blockquote>
<div><br>
</div>
<div>Can you provide some examples of pretty much all of
these? It sounds pretty reasonable, but I don't think the
API changes are visualizing for me at the moment.</div>
</div>
</div>
</blockquote>
<p>Er, I sorta feel I already have? 1 is hypothetical, but easy for
any of the invokable ones (see list in Verifier). 2 requires me
to write a patch, but should be obvious from inspecting the code
in e.g. instcombine, and 3 is covered by the patch you're
responding to. I'm not trying to be difficult here, but I'm
honestly not sure what more you want?<br></p></div></blockquote><div>I was thinking a couple of examples of "here are the kinds of API changes I expect to be able to make and this will make things safer and this is what the code will look like at the end".</div><div><br></div><div>i.e.</div><div><br></div><div>Instead of writing:</div><div><br></div><div>blah </div><div><br></div><div>I'll write</div><div><br></div><div>baz and foo->bar()</div><div><br></div><div>in the future, but that solves the long term maintainability of X because blah could have a subtle bug.</div><div><br></div><div>That make sense?</div><div> </div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div><p>
</p>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
> I see you added a bunch of people and then committed,
but I'd still like to hear the answers.<br>
<br>
Er, did you intentionally leave out the "waited for LGTM"
part? Your wording seems to imply bad faith which I don't
think is called for.<br>
<br>
</blockquote>
<div><br>
</div>
<div>I mean, I guess if all you wanted was "I need someone to
LGTM" then it's fine, but that's not what it looked like as
a proposal for discussion.</div>
</div>
</div>
</blockquote>
(combined response with next point)<br>
<blockquote type="cite">
<div dir="ltr">
<div class="gmail_quote">
<div> </div>
<blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex">
If you think this is rushed and needs further discussion,
I'm happy to revert while that discussion happens.<br>
<br>
</blockquote>
<div><br>
</div>
<div>Feels a little rushed. Had less than 4 hours to even
think about getting to it. <br>
</div>
</div>
</div>
</blockquote>
<p>Reverted in 4824d876f0 while this discussion happens.</p>
<p>I think you're misrepresenting this a bit. I put out a proposal,
that did include a question to sanity check with others. I got no
response for two weeks. I directly added several parties known to
be interested in big picture API questions. I got a single
response which seemed to endorse said proposal with an explicit
LGTM. You raised concerns, I responded promptly. This seems all
around reasonable and normal practice to me. </p></div></blockquote><div>That's fine, I would have just expected for something that seemed like a proposal to wait at least 24 hours after making people aware of it :) No harm done though!</div><div><br></div><div>-eric <br></div></div></div>