[llvm] r187283 - Reimplement isPotentiallyReachable to make nocapture deduction much stronger.

[Chris Lattner]

On Jul 29, 2013, at 5:38 PM, Nick Lewycky <nlewycky at google.com> wrote:

> On 27 July 2013 09:11, Chris Lattner <clattner at apple.com> wrote:
> On Jul 26, 2013, at 6:24 PM, Nick Lewycky <nicholas at mxc.ca> wrote:
>> URL: http://llvm.org/viewvc/llvm-project?rev=187283&view=rev
>> Log:
>> Reimplement isPotentiallyReachable to make nocapture deduction much stronger.
>> Adds unit tests for it too.
>> 
>> Split BasicBlockUtils into an analysis-half and a transforms-half, and put the
>> analysis bits into a new Analysis/CFG.{h,cpp}. Promote isPotentiallyReachable
>> into llvm::isPotentiallyReachable and move it into Analysis/CFG.
> 
> Random unit test question for you:
> 
>> +TEST_F(IsPotentiallyReachableTest, SameBlockNoPath) {
>> +  ParseAssembly(
>> +      "define void @test() {\n"
>> +      "entry:\n"
>> +      "  bitcast i8 undef to i8\n"
>> +      "  %B = bitcast i8 undef to i8\n"
>> +      "  bitcast i8 undef to i8\n"
>> +      "  bitcast i8 undef to i8\n"
>> +      "  %A = bitcast i8 undef to i8\n"
>> +      "  ret void\n"
>> +      "}\n");
>> +  ExpectPath(false);
>> +}
> 
> I understand the value of unit tests for stuff like this, but dumping swaths of LLVM IR into them seems really weird to me.  Why not just express these tests in terms of the benefit, i.e., cases that make nocapture inference work better?  Then they can be written as .ll files like most of our testsuite.
> 
> I wish I could test just the algorithm in the abstract. If I could do it with no IR at all I would, but I need to test the interaction with DominatorTree and LoopInfo, so I don't have a choice there. The test must have IR.
> 
> The effect that it has on nocapture is a bit distant, it's not quite as bad as writing a test for SmallVector by looking at its impact on the IR, but it's down that vein. Such a test would be fragile, first of all. Secondly, it's not easy to write IR which would trigger the exact isPotentiallyReachable calls that I want. Third, I want to test the four combinations of whether domtree and loopinfo are present, and the only caller now always provides domtree and never loopinfo. (I realize there's only one caller in-tree, but I have written others and have just been waiting for this to get in so that I can land them. Those will come with matching IR tests.)

Very lame, but ok I guess.  If it were me, I'd lean towards just not having a unit test for this.  You're shifting risk that the algorithm may regress to pain in updating this test as APIs evolve.

-Chris

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20130729/bf90d070/attachment.html>