[PATCH] D22071: Correct ordering of loads/stores.

Matt Arsenault via llvm-commits llvm-commits at lists.llvm.org
Wed Jul 6 23:55:39 PDT 2016


arsenm added inline comments.

================
Comment at: test/Transforms/LoadStoreVectorizer/AMDGPU/insertion-point.ll:12
@@ -12,2 +11,3 @@
+; CHECK: %w = add i32 %y, 9
 ; CHECK: %foo = add i32 %z, %w
 define void @insert_load_point(float addrspace(1)* nocapture %a, float addrspace(1)* nocapture %b, float addrspace(1)* nocapture readonly %c, i64 %idx, i32 %x, i32 %y) #0 {
----------------
jlebar wrote:
> arsenm wrote:
> > jlebar wrote:
> > > > It looks to me it is intentionally testing an implementation detail ("insert_load_point").
> > > 
> > > Looks like it to me, too.  When we committed the original patches, we all agreed that we wouldn't act with a bias towards the existing code, since we committed with existing unresolved issues.  I think this should count under that rubric.
> > > 
> > > That is, can we fix the test so it no longer tests an implementation detail?  I suppose you don't need to do that in this patch if you don't want.
> > The pass should still have an expectation for where the instructions will be inserted relative to the originals, I think a test ensuring this is useful
> > The pass should still have an expectation for where the instructions will be inserted relative to the originals
> 
> I guess I am OK with this if we can articulate in the test file or the cpp file exactly what is the rule that we expect applies to our output.  If we cannot articulate a rule, and instead we're just checking that the pass does what it currently does, I do not think that is a good test.  The reason is that, without an articulation of the rule, if the test fails, we have no way to tell whether there's a bug or if the test just needs to be changed.
> 
> (And if we can articulate a rule, it should go without saying that, inasmuch as reasonable, the test should check only for adherence to the rule, ignoring other ancillary properties of the output.)
Test should generally have a comment explaining what they are testing anyway. This change was mostly why I added this test in the first place. If something is changing any behavior of the pass, a test should capture this. I don't understand the concern about wondering if it's a bug or the test needs update, the point of having the test is you have to look at the test output changes to verify that it is still correct


http://reviews.llvm.org/D22071





More information about the llvm-commits mailing list