<html>
<head>
<meta http-equiv="Content-Type" content="text/html; charset=UTF-8">
</head>
<body>
<p><br>
</p>
<div class="moz-cite-prefix">On 4/20/21 3:54 PM, Eric Christopher
wrote:<br>
</div>
<blockquote type="cite"
cite="mid:CALehDX42eDrVPBG1sfCdk=-WwDyWqef4fR7Q9oBW+BMugpauRQ@mail.gmail.com">
<meta http-equiv="content-type" content="text/html; charset=UTF-8">
<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"
moz-do-not-send="true">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" moz-do-not-send="true">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"
moz-do-not-send="true">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>
<p>As an example, consider the current code structure from
instcombine.</p>
<p>From visitCallInst:</p>
<p> IntrinsicInst *II = dyn_cast<IntrinsicInst>(&CI);<br>
if (!II) return visitCallBase(CI);<br>
...<br>
Intrinsic::ID IID = II->getIntrinsicID();<br>
switch (IID) {<br>
case Intrinsic::objectsize:<br>
... (sometimes uses dyn_cast<IntrinsicInst> for argument
matching)</p>
<p>From visitCallBase:</p>
<p> // Handle intrinsics which can be used in both call and invoke
context.<br>
switch (Call.getIntrinsicID()) {<br>
case Intrinsic::experimental_gc_statepoint: {<br>
GCStatepointInst &GCSP =
*cast<GCStatepointInst>(&Call);<br>
.. etc<br>
}<br>
</p>
<p>Would become:<br>
</p>
<p>visitiIntrinsicInst <br>
(using the InstVisitor dispatch point enabled by this change, and
thus no separate explicit cast)</p>
<p> Intrinsic::ID IID = II->getIntrinsicID();<br>
switch (IID) {<br>
case Intrinsic::objectsize:<br>
... (sometimes uses dyn_cast<IntrinsicInst> for argument
matching)<br>
case Intrinsic::experimental_gc_statepoint: {<br>
GCStatepointInst &GCSP =
*cast<GCStatepointInst>(&Call);<br>
.. etc<br>
}<br>
</p>
<p>That seems cleaner to me. We get similar cleanup in
SelectionDAG.</p>
<p>I do want to acknowledge that we can get a good amount of that
unification by just using CallBase in place of IntrinsicInst, but
well, at that point what's the point of having intrinsic inst?<br>
</p>
<p><br>
</p>
<p>An example where this makes code slightly worse (from FastISel in
original patch)</p>
<p> case Intrinsic::xray_customevent:<br>
return selectXRayCustomEvent(cast<CallInst>(II));</p>
<p>The casts to CallInst are new. This is a call where having a
distinction between InstrinicBase and IntrinsicInst (as suggested
in another response) might help.</p>
<p>I'll note in passing that several cases in the original patch
where we cast to CallInst, we could instead change the interface
to return IntrinsicInst or CallBase and everything just works. I
was trying to minimize diff size, which makes the code impact look
slightly worse than it'd end up being.<br>
</p>
<p></p>
<blockquote type="cite"
cite="mid:CALehDX42eDrVPBG1sfCdk=-WwDyWqef4fR7Q9oBW+BMugpauRQ@mail.gmail.com">
<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">
<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>
</blockquote>
</body>
</html>