
<html><head><meta http-equiv="Content-Type" content="text/html charset=us-ascii"></head><body style="word-wrap: break-word; -webkit-nbsp-mode: space; -webkit-line-break: after-white-space; "><br><div><div>On Dec 18, 2012, at 2:27 PM, Chandler Carruth <<a href="mailto:chandlerc@google.com">chandlerc@google.com</a>> wrote:</div><br class="Apple-interchange-newline"><blockquote type="cite"><div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr"><div class="gmail_default" style="">On Tue, Dec 18, 2012 at 2:07 PM, Michael Ilseman <span dir="ltr"><<a href="mailto:milseman@apple.com" target="_blank" class="cremed">milseman@apple.com</a>></span> wrote:<br>

</div><div class="gmail_extra"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">Attached are patches for fast-math support for IntrinsicInst.</blockquote>

<div><br></div><div style="">I have one specific concern:</div><div><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"> This includes bitcode changes to separate out IntrinsicInsts from CallInsts.<br>

</blockquote><div style=""><br></div><div style=""><snip></div><div style=""><br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">

Previously, IntrinsicInsts were handled as CallInsts. However, intrinsics can now have fast-math flags, and thus merit a separate bitcode encoding. This change adds FUNC_CODE_INST_INTRINSIC for intrinsic calls, separates them out from general calls, and adds encoding and decoding of fast-math flags. Backwards compatibility is retained. Adds parsing fast-math flags for IntrinsicInsts, and more tests.<br>

</blockquote><div><br></div><div style="">This seems like a terrible consequence of using flags instead of metadata. Why is this a good thing to do?</div><div style=""><br></div></div></div></div></div></blockquote><div><br></div><div>What is this terrible consequence? Why is it terrible to have another bitcode for intrinsics? They already have their own class at the API level. I can certainly just keep reusing the bitcode for CallInst, find some place to stick the bits in, and throw an error if the callee doesn't resolve to an intrinsic, but is that preferable? If anything, that approach might leave to a SWITCH_INST_MAGIC-eque versioning.</div><div><br></div><div>As far as using metadata, that discussion has existed in multiple incarnations, the most recent of which is:</div><div><a href="http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-November/055764.html">http://lists.cs.uiuc.edu/pipermail/llvmdev/2012-November/055764.html</a></div><div><br></div><br><blockquote type="cite"><div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div style="">I think we either need to either not have fast math markers on intrinsics or provide metadata alternatives to the flags so we can use that pre-existing general mechanism for extending the set of information we have about a particular group of instructions.</div>

<div style=""><br></div></div></div></div></div></blockquote><div><br></div><div>We are reusing the pre-existing mechanism for extending the set of information we have about a particular group of instructions. There is nothing in the code before these patches that stops IntrinsicInst from having fast-math flags at the API level or internal representation, there just was no way to encode/decode them (or parse them from text). </div><div><br></div><div>This still uses FPMathOperator as the internal representation, and nothing meaningful is introduced to change that. If you're referring to the "|| isa<IntrinsicInst>(I)" change from Operator.h, that diff can just be completely dropped from these patches without consequence (in fact, I'll do so as it's confusing). </div><div><br></div><blockquote type="cite"><div style="font-family: arial, helvetica, sans-serif; font-size: 10pt"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div style="">Making calls to intrinsic functions less like any other call adds complexity to the entire IR just to support a few intrinsics and a few special-purpose optimizations.</div></div></div></div>

</div>

</blockquote></div><br><div>Calls to intrinsic functions are already different from normal calls. Users already switch on the IntrinsicID in order to attempt optimization based on their known semantics. The internal representation is already a valid FPMathOperator, and intrinsics are good matches for fast-math flags as the optimizer knows their definition and can preserved semantic definedness.</div><div><br></div><div><br></div><div>Here's some patches that remove the confusing changes to Operator.h. The only changes are to the encoder/decoder, and the text parser.</div><div><br></div><div><br></div><div></div></body></html>