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

[Justin Lebar via llvm-commits]

jlebar 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 {
----------------
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.)


http://reviews.llvm.org/D22071