<div dir="ltr"><div class="gmail_extra"><div class="gmail_quote">On Tue, Dec 23, 2014 at 12:41 PM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div><div class="h5"><br><div><blockquote type="cite"><div>On Dec 23, 2014, at 10:54 AM, Bob Wilson <<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>> wrote:</div><br><div><div style="word-wrap:break-word"><br><div><blockquote type="cite"><div>On Dec 23, 2014, at 10:41 AM, Chris Lattner <<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>> wrote:</div><br><div><div style="word-wrap:break-word">On Dec 23, 2014, at 10:28 AM, Chris Bieneman <<a href="mailto:beanz@apple.com" target="_blank">beanz@apple.com</a>> wrote:<br><div><blockquote type="cite"><div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px"><blockquote type="cite"><div><div style="word-wrap:break-word"><div><blockquote type="cite"><div><div style="word-wrap:break-word"><div>It should be straight-forward to have something like LLVMInitializeX86Target/RegisterTargetMachine install the intrinsics into a registry.</div></div></div></blockquote><br></div><div>I tried doing that a few years ago. It’s not nearly as easy as it sounds because we’ve got hardcoded references to various target intrinsics scattered throughout the code.</div></div></div></blockquote><br></div><div style="font-family:Helvetica;font-size:12px;font-style:normal;font-variant:normal;font-weight:normal;letter-spacing:normal;line-height:normal;text-align:start;text-indent:0px;text-transform:none;white-space:normal;word-spacing:0px">I was just writing to say exactly this. There are a number of ways we could work toward something like this. I’m completely in favor of a world where Intrinsics are properties of the targets and don’t leach out, however today they do in a lot of places.</div></div></blockquote><div><br></div>What are the specific problems here?  Anything that does an equality comparison with the IntrinsicID can be changed to do strcmp with the name.  That would handle the one-off cases like InstCombiner::SimplifyDemandedUseBits in InstCombine.</div></div></div></blockquote><blockquote type="cite"><div><div style="word-wrap:break-word"><div><br></div><div>The other cases in InstCombine could be handled similarly, but may be better handled by adding a intrinsic behavior query APIs to the intrinsic registry, or would be better handled (eventually) by adding new attributes to the intrinsics themselves.</div></div></div></blockquote><br></div><div>I don’t think there’s anything fundamentally difficult, but it’s a big change. For example:</div><div><br></div><div>$ git grep Intrinsic:: | wc<br>    3364   12286  281078<br><br></div><div>The vast majority of those 3,364 lines have hardcoded references to specific intrinsics. Many of them are used in contexts where you can’t easily insert a strcmp (e.g., case values in large switch statements, or worse, the m_Intrinsic PatternMatch templates).</div></div></div></blockquote></div><br></div></div><div>I don’t find this convincing.  It should be simple to introduce a new m_Intrinsic PatternMatch template that takes a string.  The switches are also straight-forward.  In BasicAA, for example, instead of:</div><div><br></div><div><div style="margin:0px;font-size:10px;font-family:Monaco">   switch (II->getIntrinsicID()) {</div><div style="font-family:Monaco;font-size:10px;margin:0px">    default: break;</div><div style="margin:0px;font-size:10px;font-family:Monaco">    case Intrinsic::memset:</div><div style="margin:0px;font-size:10px;font-family:Monaco">    case Intrinsic::memcpy:</div><div style="margin:0px;font-size:10px;font-family:Monaco">    case Intrinsic::memmove: {</div></div><div style="margin:0px;font-size:10px;font-family:Monaco">    ...</div><div style="margin:0px;font-size:10px;font-family:Monaco"><div style="margin:0px"><div style="margin:0px">    case Intrinsic::arm_neon_vld1: {</div><div style="margin:0px">      assert(ArgIdx == 0 && "Invalid argument index");</div><div style="margin:0px">      assert(Loc.Ptr == II->getArgOperand(ArgIdx) &&</div><div style="margin:0px">             "Intrinsic location pointer not argument?");</div><div style="margin:0px">      // LLVM's vld1 and vst1 intrinsics currently only support a single</div><div style="margin:0px">      // vector register.</div><div style="margin:0px">      if (DL)</div><div style="margin:0px">        Loc.Size = DL->getTypeStoreSize(II->getType());</div><div style="margin:0px">      break;</div><div style="margin:0px">    }</div></div></div><div style="margin:0px;font-size:10px;font-family:Monaco"><div style="margin:0px">}</div><div style="margin:0px"><br></div><div style="margin:0px"><span style="font-family:Helvetica;font-size:12px">Just change this to:</span></div><div style="margin:0px"><span style="font-family:Helvetica;font-size:12px"><br></span></div><div style="margin:0px"><div style="font-family:Helvetica;font-size:12px"><div style="margin:0px;font-size:10px;font-family:Monaco">   switch (II->getIntrinsicID()) {</div><div style="font-family:Monaco;font-size:10px;margin:0px">    case Intrinsic::memset:</div><div style="margin:0px;font-size:10px;font-family:Monaco">    case Intrinsic::memcpy:</div><div style="margin:0px;font-size:10px;font-family:Monaco">    case Intrinsic::memmove: {</div></div><div style="margin:0px">    ...</div><div style="margin:0px"><div style="margin:0px"></div></div></div></div><div><div style="font-family:Monaco;font-size:10px;margin:0px">    default:</div><div style="font-family:Monaco;font-size:10px;margin:0px">      if (II->getName() == “llvm.arm.neon.vld1”) {</div><div style="font-family:Monaco;font-size:10px;margin:0px">        same stuff as above</div><div style="font-family:Monaco;font-size:10px;margin:0px">      }</div><div style="font-family:Monaco;font-size:10px;margin:0px">    break;</div></div></div></blockquote><div><br></div><div><br></div><div>If we're going to talk about what the right long-term design is, let me put out a different opinion. I used to be somewhat torn on this issue, but this discussion and looking at the particular intrinsics in question, I'm rapidly being persuaded.</div><div><br></div><div>We shouldn't have any target specific intrinsics. At the very least, we shouldn't use them anywhere in the front- or middle-end, even if we have them.</div><div><br></div><div>Today, frontends need to emit specific target instrinsics *and* have the optimizer be aware of them. I can see a few reasons why:</div><div><br></div><div>1) Missing semantics -- the IR may not have *quite* the semantics desired and provided by the target's ISA.</div><div>2) Historical expectations -- the GCC-compatible builtins are named after the instructions, and the target independent builtins lower to intrinsics so the target-specific ones should too.</div><div>3) Poor instruction selection -- we could emit the logic as boring IR, but we fail to instruction select that well, so as a hack we emit the instruction directly and teach the optimizer to still optimize through it.</div><div><br></div><div>If we want to pursue the *right* design, I think we should be fixing these three issues and then we won't need the optimizer to be aware of any of this.</div><div><br></div><div>Fixing #1) We just need to flush these out and define target independent intrinsics. The number of these is relatively small, and not likely to be a burden. I'm looking at you cvtss2si.</div><div><br></div><div>Fixing #2) I think this just requires a giant pile of work to re-target builtins and other things that people expect the optimizer to operate on into proper IR. Yes, it's hard, but it is the right design. With x86 we're already about 90% of the way there. My understanding is that ARM has gone the other direction. While unpleasant, I think we have to make a choice: either the builtin doesn't get optimized or it gets lowered to "normal" IR.</div><div><br></div><div>Fixing #3) Yes, we need better instruction selection. We have always needed better instruction selection. If there are truly critical needs that aren't met, we can even achieve this through much more localized hacks *in the backend* that form specific intrinsics to "pre select" instructions that we know are important. While just as hacky and gross, this seems like a much better tradeoff to make than the current approach.</div><div><br></div><div>In the end, we don't have target-specific intrinsics in the middle end at all. Then we get to decide on whether it is worth our time to provide a really efficient target-specific *encoding* of these operations, or if we should just lower to the target specific encoding we already have: inline assembly. I don't have strong feelings about this either way.</div><div><br></div><div><br></div><div><br></div><div>OK, so I think that is the right design, but I also completely agree with Chris B here -- it is *way more work* than is really called for to address a very narrow, targeted problem that WebKit has: binary size.</div><div><br></div><div>But I also agree with Chris L's point here:</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div style="word-wrap:break-word"><div>Spreading ifdefs through the codebase would be really unfortunate.</div></div></blockquote><div><br></div><div>I just don't think this is necessary. And I don't think relying on a fragile frontend-based "optimization" to achieve the same effects is really great either.</div><div><br></div><div><br></div><div>The huge code in IR/Function.cpp.o is from *terrible* code being emitted by tablegen. We should just change tablegen to emit something smaller. This will get most of the size win, and will get it even when the target *is* compiled in!</div><div><br></div><div>In particular, if we switch to a sorted table of intrinsic names, change the *lookup* of names to use the same sorted table so we share the memory, and do a binary search, I think the code size will be essentially free.</div><div><br></div><div>Once we address that, Chris B can use the somewhat ugly #ifdef's or his clang attribute patch to look for other hotspots where we burn size on intrinsics. I bet a few more could be fixed by similar effort.</div><div><br></div><div>If the code size is *still* a problem, we could could start factoring things incrementally into target independent intrinsics and removing the target specific ones which might help incrementally move us toward the "right design" (IMO).</div><div><br></div><div>Then if someone is really motivated (which would be cool) to do all the hard work to really move us over to the right design, we would have lost nothing, there would be essentially no more technical debt than there is today, and WebKit and others won't have to wait for their binary size improvements. </div><div><br></div><div>-Chandler</div></div></div></div>