<div dir="ltr">Dave and I chatted about this for a bit...<br><div class="gmail_extra"><br><div class="gmail_quote">On Wed, Sep 17, 2014 at 6:18 PM, Eric Christopher <span dir="ltr"><<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>></span> wrote:<br><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><br>
+ virtual const TargetSubtargetInfo *getSubtargetImpl(const Function *) const {<br>
+ return getSubtargetImpl();<br></blockquote><div><br></div></div></div><div>So what's left to do to make this actually do the real work of picking a Subtarget based on the Function? I'm just curious about where that'll fit in your migration plan?<br><br></div></div></div></div></blockquote><div><br></div></span><div>I was going to fix up all of the occurrences of getting a subtarget without a function first and then actually pick the function. That gives me a bunch of no functional change patches to get everything going.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div>Once this function is actually implemented, would it be possible to test just this one change (& then test each subsequent feature as you port it over)? </div><div><div><div> </div></div></div></div></div></div></blockquote><div><br></div></span><div>There are no functional changes in this patch.</div><span><div> </div></span></div></div></div></blockquote><div><br></div><div>We've agreed that if I did the TargetMachine subtarget creation patch first then there could be a visible change if I modified testcases to have a default module and moved the cpu or target features down to the individual functions in the test I could probably test this. It would involve a lot of testcase modification and probable duplication to do this at the level of function call granularity, i.e. test every API that goes through in a separate way. I think we're going to compromise that I'll see what I can do to add the TargetMachine subtarget creation stuff in now and then work on testing functionality as I can.</div><div><br></div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><span><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
- void getUnrollingPreferences(Loop *L,<br>
+ void getUnrollingPreferences(const Function *F, Loop *L,<br></blockquote><div><br></div></div></div><div>Would it make more sense for this to take the TargetSubtargetInfo directly? A more constrained API (makes it clear what this function is using - just the subtarget info, not other things with the Function) and potentially fewer Subtarget lookups (which are zero cost today, but later will become at least some kind of map lookup/lazy construction/etc?).</div><div><div><div> </div></div></div></div></div></div></blockquote><div><br></div></span><div>Possibly. I thought about limiting it here, but the problem is that each particular target could have it's own desires and I was hoping for a more uniform API. It's definitely something that could be adjusted though.</div><span><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_extra"><div class="gmail_quote"><div><div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
-void PPCTTI::getUnrollingPreferences(Loop *L, UnrollingPreferences &UP) const {<br>
- if (ST->getDarwinDirective() == PPC::DIR_A2) {<br>
+void PPCTTI::getUnrollingPreferences(const Function *F, Loop *L,<br>
+ UnrollingPreferences &UP) const {<br></blockquote><div><br></div></div></div><div>And in places like this, if you passed the Subtarget, you could even call it "ST" and then the body of the function wouldn't need any changes at all. (granted, shadowing is subtle stuff... so I can understand if that's not ideal - but given the ultimate future will end up removing the member variable, it might be an acceptable intermediate state to reduce typing/churn)</div></div></div></div></blockquote><div><br></div></span><div>Right. And this would be why it might work. I.e. instead of Function, take Subtarget since that'll be what everything uses ultimately to get the various target data structures. I have no ultimate preference here other than I will likely have to cast that subtarget no matter what to be the type of the actual TargetSubtargetInfo that the target wants (I.e. PPCSubtargetInfo).</div><div><br></div><div>That said, it doesn't necessarily lose me anything.</div></div></div></div></blockquote><div><br></div><div>Having thought about this I think I'm going to go with it. I'll still have to do the specific casts but it will save some additional lookups and compile time. It might be premature optimization, but the API isn't any worse with it so let's do it.</div><div><br></div><div>-eric</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
</blockquote></div><br></div></div>