On Tue, Mar 31, 2009 at 9:04 PM, Chris Lattner <span dir="ltr"><<a href="mailto:clattner@apple.com" target="_blank">clattner@apple.com</a>></span> wrote:<br><div class="gmail_quote"><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div><div><div>On Mar 30, 2009, at 1:34 PM, Misha Brukman wrote:</div><blockquote type="cite"><div class="gmail_quote"><div>I'm proposing we test passes in C++ instead of a shell script that calls grep.</div>
</div></blockquote></div><div>What is the real value of doing this? The costs I see are 1) increased link time and 2) more dependence on the API (more to update when an API changes).</div></div></div></blockquote><div><br>
I address your issue (1) in #3 below. I accept your point (2) as valid, but based on personal experience, although it's a non-zero effort, it's not as painful as you imagine, and coupled with the benefits below, I think it's worthwhile. Consider it a tradeoff (of CPU time for human time), rather than simply an added cost.<br>
<br>Let's look at the benefits:<br><br>1) Tests can be written at any level of the LLVM API, so you can write tests to verify a single function, check that a class modifies its state properly (even private state), verify a pass output, or verify that llc or opt does everything end-to-end correctly. For instance, let's say we have a transformation that's dependent on an analysis, and say we have a test case where the transformation should have no effect.<br>
<br>We could test with the opt/diff to make sure the output is the same as input, but that's not really testing what we think we are, because it's testing the interaction of the two, so a bug in one can mask a bug in another. In other words, there are many code paths that can "return false" along the way, and cause the transformation not to be applied, but do you really know WHICH one is responsible?<br>
<br>With a C++ test, you can test the analysis pass separately and test its outputs on one test, and then in another test, inject a Mock (or Fake) analysis object with known values, and see how the pass behaves. This lets you test each piece of code independently, and prevents complex interactions from messing with your tests.<br>
<br>2) Although LLVM may have high line-coverage via large tests, we don't know what code path coverage it has, which is also important. If we can only test at the granularity of a pass, there are 2**N code paths, and it's hard to contrive .ll inputs that will cause function foo() to be called with inputs bar and quux (also makes debugging more difficult than it should be, see #3 below). With a single test, you can synthesize exactly what you want the inputs to be, thus avoiding single-stepping through a debugger when you don't need to. So instead of writing, say O(2**N) tests, you can write O(N) tests (in number, not runtime!) to get much better path coverage.<br>
<br>3) Currently, when something is broken, bugpoint is introduced to narrow down
the test case to something small, and then the developer steps through the code
with a debugger will (hopefully) to find which part of which data structure isn't being
updated properly. I have personally been spending *MUCH* less time interacting with debuggers thanks to testing -- instead of setting a break point and manually verifying state at function entry/exit, I can set up the test scenario, and call EXPECT() to verify what I think it should be, and let it run! I'll quickly know if it's a problem or a red herring. This works for any granularity of code, and is really valuable.<br>
<br>Without tests, each developer manually repeats the debugging via "Oh, I think it's caused when X happens and Y interacts with Z!". If you have a test for that that you commit to the repository, anyone who thinks it's broken can just re-run the test and see that no, that guess was wrong, try again.<br>
<br>This will save everyone a LOT of time, and given that developer time is more valuable than CPU time, I would rather have the machine compile/link/run tests (which is inactive time for me -- I can switch to working on something else), than firing up my debugger to single-step through code and inspect complex data structures that could and should be verified automatically, given my specification (in the form of a test).<br>
<br>4) There are other things we cannot test via opt/grep directly, or even llvmirgrep:<br>(a) anything that doesn't have text-based output to grep through, e.g.<br> (i) analysis passes (alias analysis, etc.)<br>
(ii) PassManager (e.g., verify pass ordering in the presence of FunctionPasses and ModulePasses, etc. that's independent of code being run)<br>
(b) things that read or emit data are only tested indirectly: bitcode reader/writer, TableGen, code emitters, etc.<br><br>Bottom line -- I see at least 2 open bugs on Andersen's that would benefit. How
about we expose its class in a header to be able to write tests and see
how it goes? And there is a bug for a pass that seems to run out of memory that might also benefit. And next time there's some difficult-to-debug problem
that you see as similar to the above, please file a bug and assign it to me, I'll
see what I can do for it with some targetted tests. How does that sound?<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div><div><blockquote type="cite"><div class="gmail_quote"><div><span>How else can we check the correctness of a pass, which involves pattern-matching multiple instructions? Grep can only look at a single line at a time, and doesn't have the semantic knowledge of what's an instruction and what's an operand, whereas we have all this information in C++.</span></div>
</div> </blockquote></div></div><div>I think the answer is that we need to build *one* small "llvmirgrep" tool that does this, which could be used by many different tests.</div><div></div></div></blockquote><div>
<br>This is a tool that doesn't exist yet, which means it will require time and effort to design and implement it, as well as (dare I say it?) test it. But LLVM already has something like this in C++, llvm::PatternMatch. I was thinking this could be used to pattern-match on LLVM expressions to verify the transformations.<br>
</div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div>I don't know exactly the syntax for it, but I think it should be something like the tool we use in clang. In clang, we write tests for diagnostics like this:</div>
<div><div></div></div></div></blockquote><div>[snip example]<br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;"><div><div><div>In this case, the tool is built into clang, but it doesn't need to be. The -verify mode basically runs the compiler as normal, but captures the generated diagnostics to a buffer. It then scans the .c file for the "expected-" comments and "diffs" the generated and expected diagnostics.</div>
</div></div></blockquote><div><br>Sure, this works because the diagnostic is on the same line as the error, i.e., you don't need to pattern-match multiple lines.<br><br></div><blockquote class="gmail_quote" style="border-left: 1px solid rgb(204, 204, 204); margin: 0pt 0pt 0pt 0.8ex; padding-left: 1ex;">
<div><div>
<div>If we had an llvmirgrep tool, then we could do something like this:</div><div><br></div><div><div>; RUN: llvm-as < %s | opt -instcombine | llvm-irgrep %s</div><div><br></div><div>define i32 @test(float %f) {</div>
<div> ; irgrep {%t1 = bitcast float %f to i32}</div><div> ; irgrep {ret i32 %t1}</div><div> %tmp7 = insertelement <4 x float> undef, float %f, i32 0 </div><div> %tmp17 = bitcast <4 x float> %tmp7 to <4 x i32> </div>
<div> %tmp19 = extractelement <4 x i32> %tmp17, i32 0</div><div> ret i32 %tmp19</div><div>}</div><div><br></div><div>Which would be smart enough to require that those two instructions be in that basic block of that function, and it would do a fuzzy match to ignore the fact that the generated code names the first instruction "%tmp19".</div>
</div></div></div></blockquote><div><br>The syntax will get complex when you want to have a pass that deletes an instruction, or moves it outside of a loop, or breaks up a loop header, etc.<br><br>What you want is structural equivalence, which is what I think llvm::PatternMatch would do for instructions, or llvm-irgrep will re-implement. If it re-uses that API, we should just write tests with it and extend when necessary. If it's re-creating the API, then we're duplicating effort.<br>
<br>Since we're hand-waving, let me hand wave some testing code.<br><br>TEST(InstCombine, BitCast) {<br> // .. create the IR using IRBuilder or other method<br> Function *F = ...;<br> <br> InstCombine.runOnFunction(F);<br>
<br> // We want to see a single BasicBlock with this code:<br>
// %y = bitcast float %x to i32<br>
// ret i32 %y<br>
EXPECT_EQ(1, F->numBasicBlocks());<br> const BasicBlock &BB = F->front();<br> Value *X, *Y;<br> ASSERT_TRUE(llvm::PatternMatch::match(BB.getTerminator(), m_BitCast(m_Value(X), m_value(Y))));<br> ASSERT_EQ(Type::float, X->type());<br>
ASSERT_EQ(Type::i32, Y->type());<br>
}<br><br>Alternatively, if we write a test helper function to compare equality of two BasicBlocks, we can just do:<br> <br> BasicBlock *ExpectedBB = ...;<br> EXPECT_TRUE(AreBasicBlocksEqual(*ExpectedBB, F->front()));<br>
<br>after we set up the ExpectedBB with the code we want it to have. It sounds like llvm-irgrep will have to have this functionality anyway to run your example, but here, you can compare anything structurally, e.g. the flow graph, or even an entire Module, which will probably be hard to express in text for llvm-irgrep.<br>
<br>Misha<br>
</div></div>