Just a few things here... <div><br></div><div><div>+ CGF.EmitBranchOnBoolExpr(E->getConfig(), ContBlock, ConfigOKBlock, 0);</div></div><div><br></div><div>Entirely up to you, but I find that commenting options like the 0 there with "Initial value" or something is useful when you're not passing an obvious argument along that has a name.</div>
<div><br></div><div><div><div>+ if (CondExprBool)</div><div>+ Cnt.beginRegion(Builder);</div></div><div><br></div><div>I'd probably want a comment above the conditional here.</div><div><br></div><div><div>
+ PGO.setCurrentRegionCount(0);</div></div><div><br></div><div>Perhaps resetCurrentRegionCount to do the explicit 0? It's more obvious what's going on then.</div><div><br></div><div><div>+ Cnt.adjustFallThroughCount();</div>
</div><div><br></div><div>This is weird - I'll mention it later :)</div><div><br></div><div><div>+ uint64_t Total = CaseCnt.getCount() - CaseCnt.getParentCount();</div><div>+ unsigned NCases = Range.getZExtValue() + 1;</div>
<div>+ uint64_t Weight = Total / NCases, Rem = Total % NCases;</div><div>+ for (unsigned I = 0; I != NCases; ++I) {</div><div>+ if (SwitchWeights)</div><div>+ SwitchWeights->push_back(Weight + (Rem ? 1 : 0));</div>
<div>+ if (Rem)</div><div>+ Rem--;</div><div> SwitchInsn->addCase(Builder.getInt(LHS), CaseDest);</div></div><div><br></div><div>Should comment what you're doing here.</div><div><br></div><div><div>
+ (*SwitchWeights)[0] += ThisCount;</div></div><div><br></div><div>This seems awkward.</div><div><br></div><div><div>+ llvm::BasicBlock *SkipCountBB = 0;</div><div>+ if (CGM.getCodeGenOpts().ProfileInstrGenerate) {</div>
<div>+ SkipCountBB = createBasicBlock("skipcount");</div><div>+ EmitBranch(SkipCountBB);</div><div>+ }</div></div><div><br></div><div>Probably want a comment of why you're emitting this block.</div><div>
<br></div><div><div>+ const SwitchCase *Case = S.getSwitchCaseList();</div><div>+ uint64_t DefaultCount = 0;</div><div>+ unsigned NumCases = 0;</div><div>+ for (; Case; Case = Case->getNextSwitchCase()) {</div>
</div><div><br></div><div>No reason to have Case outside of the loop as far as I can tell?</div><div><br></div><div><div>- llvm::BasicBlock *FalseBlock);</div><div>+ llvm::BasicBlock *FalseBlock, uint64_t TrueCount);</div>
</div><div><br></div><div>Comment what TrueCount is above.</div><div><br></div><div><div><br></div></div><div><div>+ void emitCounterIncrement(CGBuilderTy &Builder, unsigned Counter) {</div><div>+ if (!CGM.getCodeGenOpts().ProfileInstrGenerate)</div>
<div>+ return;</div><div>+ llvm::Value *Addr =</div><div>+ Builder.CreateConstInBoundsGEP2_64(RegionCounters, 0, Counter);</div><div>+ llvm::Value *Count = Builder.CreateLoad(Addr, "pgocount");</div>
<div>+ Count = Builder.CreateAdd(Count, Builder.getInt64(1));</div><div>+ Builder.CreateStore(Count, Addr);</div><div>+ }</div></div><div><br></div><div>Probably a bit big to have in the header.</div><div><br></div>
<div><div>+ /// Get the value of the counter with adjustments applied. Adjustments are</div><div>+ /// applied whenever control flow enters or leaves the region abnormally.</div><div>+ uint64_t getAdjustedCount() const {</div>
<div>+ assert(Adjust > 0 || (uint64_t)(-Adjust) <= Count && "Negative count");</div><div>+ return Count + Adjust;</div><div>+ }</div></div><div><br></div><div>Few things with this:</div><div>
<br></div><div>a) Adjustments seem awkward, possibly need to describe this better.</div><div>b) You use accessor functions in all of the other functions.</div><div><br></div><div><div>+ /// Get the value of the counter that was active before this one.</div>
<div>+ uint64_t getParentCount() const { return ParentCount; }</div></div><div><br></div><div>Parent count seems a bit awkward for naming. I've seen the uses through the source, but some documentation of what's going on here and why you'd want to get it would be good.</div>
<div><br></div><div><div>+ /// Get the associated break counter. Undefined if this counter is not</div><div>+ /// counting a loop.</div><div>+ RegionCounter getBreakCounter() const {</div><div>+ return RegionCounter(*PGO, Counter + 1);</div>
<div>+ }</div><div>+ /// Get the associated continue counter. Undefined if this counter is not</div><div>+ /// counting a loop.</div><div>+ RegionCounter getContinueCounter() const {</div><div>+ return RegionCounter(*PGO, Counter + 2);</div>
<div>+ }</div></div><div><br></div><div>"Undefined" boo! Any way to make it do something sane or assert?</div><div><br></div><div><div>+ void setFallThroughCount() {</div></div><div><br></div><div>Possibly "finishAdjustments" or something like that? A set that doesn't take an argument is a bit odd. The "adjustFallThroughCount()" code is also weird. I like abstracting the idea out, but I'm not sure how it should be done here.</div>
<div><br></div><div>The testcases look much better. I'm a bit astounded that you can map them to the source quite like that, but hey, awesome that you can.</div></div><div><br></div><div>Perhaps having a comment operator in the input file format would be nice so that you could comment the input files, but that's definitely an "in the future" sort of thing.</div>
<div><br></div><div>-eric</div><br><div>On Sun Jan 05 2014 at 6:34:13 PM, Eric Christopher <<a href="mailto:echristo@gmail.com">echristo@gmail.com</a>> wrote:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<p dir="ltr">Will get to this tomorrow. </p>
<div>On Jan 5, 2014 6:33 PM, "Justin Bogner" <<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>> wrote:<br type="attribution"><blockquote style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
ping...<br>
<br>
Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> writes:<br>
> Must have slipped off of my radar - especially with the holidays.<br>
> Thanks for the ping.<br>
><br>
> -eric<br>
><br>
> On Sun, Dec 29, 2013 at 8:49 PM, Bob Wilson <<a href="mailto:bob.wilson@apple.com" target="_blank">bob.wilson@apple.com</a>> wrote:<br>
>> Eric,<br>
>><br>
>> Thanks for your careful review of this. Your comments about the<br>
>> testing strategy have been especially helpful. This is an initial<br>
>> patch that is going to be revised extensively before the feature is<br>
>> finished. There are a lot of incremental changes that need to<br>
>> happen, and at least for myself, I have been unable to contribute<br>
>> any of those changes while we wait to get this initial patch<br>
>> committed. It has now been almost a month since Justin first<br>
>> submitted this patch (Dec. 1). I have carefully reviewed it twice,<br>
>> and John (the code owner) has also reviewed it. Can you please give<br>
>> an OK to let Justin commit this patch?<br>
>><br>
>> On Dec 19, 2013, at 12:37 AM, Justin Bogner <<a href="mailto:mail@justinbogner.com" target="_blank">mail@justinbogner.com</a>> wrote:<br>
>><br>
>>> Eric Christopher <<a href="mailto:echristo@gmail.com" target="_blank">echristo@gmail.com</a>> writes:<br>
>>>> I don't think we should have any executable tests in the front end at all. I<br>
>>>> think the easiest way here would be to check in an input file alongside the<br>
>>>> test file similar to how the Object tests work (an Inputs directory).<br>
>>>><br>
>>>> Thoughts?<br>
>>><br>
>>> I'm a bit leery of input files, especially since the file format for the<br>
>>> PGO stuff is explicitly in flux here. That said, writing tests the way<br>
>>> you suggest has a number of advantages and tests that only sometimes run<br>
>>> are clearly inferior.<br>
>>><br>
>>> So I went ahead and ripped out the profile-generate part, added an input<br>
>>> file for the profile-use part, and even added a test that we ignore<br>
>>> bogus data, which was impossible with the previous approach.<br>
>>><br>
>>> Doing so pointed out the problem with this change. The tests I had were<br>
>>> testing two things: generating profile data, and using it. Using an<br>
>>> input file was only the latter. That's lame, so I've added a second run<br>
>>> line that spits out IR and checks that we're incrementing the<br>
>>> appropriate counters for the various constructs.<br>
>>><br>
>>> In short, this makes the tests *way* better. They're twice as<br>
>>> complicated, but they're testing twice as much stuff. Check it out.<br>
>>><br>
>>> <0002-CodeGen-Initial-instrumentation-based-PGO-implementa.patch>_______________________________________________<br>
>>> cfe-commits mailing list<br>
>>> <a href="mailto:cfe-commits@cs.uiuc.edu" target="_blank">cfe-commits@cs.uiuc.edu</a><br>
>>> <a href="http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits" target="_blank">http://lists.cs.uiuc.edu/mailman/listinfo/cfe-commits</a>><br>
</blockquote></div>
</blockquote>