[PATCH] D39604: [DAGCombine] Move AND nodes to multiple load leaves

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: test/CodeGen/ARM/and-load-combine.ll:1
+; RUN: llc -mtriple=armv7 %s -o - | FileCheck %s
+; RUN: llc -mtriple=armv7eb %s -o - | FileCheck %s
----------------
niravd wrote:
> samparker wrote:
> > niravd wrote:
> > > Please commit the test cases separately so we can see the differences more easily in phabricator.
> > Sorry, I'm confused as to why that would make it easier?
> It's because the CHECK-NOT: lsl lines are not intuitively obvious as to why they are there (I'm still not sure why those were selected, but presumably it's important). IIUC all that matters is that originally the larger loads are being used. That's very easy to see with with a prepatch test case checking for the original code generated. I suppose additional CHECK-NOTs would work as well, but I'm not sure if that confuses the CHECK-NOT: lsl 
> 
Just a drive-by comment about codegen checks: there's a script to auto-generate the FileCheck lines at llvm/utils/update_llc_test_checks.py (and it has support for ARM triples, but not all of the ones in this test file - any help in getting more triples to work with the script is greatly appreciated!).

If you use that, then it becomes very simple to add tests with complete checking *before* you make a functional change that will change the asm. This makes it easier to review exactly what changes. It also avoids the inherent fragility of 'CHECK-NOT' (imagine that someone has an 'lsl' string in the path to this test file and FileCheck sees that path string).


https://reviews.llvm.org/D39604