[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


More information about the llvm-commits mailing list