[PATCH] D38232: [MergeICmps] Disable mergeicmps if the target does not want to handle memcmp expansion.

Clement Courbet via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Oct 4 09:22:41 PDT 2017


courbet added a comment.

Thanks, PTAL.



================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:597-599
 class MergeICmps : public FunctionPass {
  public:
   static char ID;
----------------
spatel wrote:
> The indentation is non-standard; you should run clang-format before this patch to cleanup.
I don't know why, but clang-format keeps indenting "public" and "private" to one space (even after I fix these manually). Anything I missed ?


================
Comment at: test/Transforms/MergeICmps/X86/pair-int32-int32.ll:1
-; RUN: opt -mergeicmps -S -o - %s | FileCheck %s
+; RUN: opt -mergeicmps -mtriple=x86_64-unknown-unknown -S -o - %s | FileCheck %s --check-prefix=X86
 
----------------
spatel wrote:
> Here and in the other test files, you should prefer to read the input from file rather than specify a file name as param:
> ; RUN: opt < %s ...
> 
> The benefit is that if you let 'opt' know the filename, it could potentially print that name and the path to that file as part of some debug string. Now if that string happens to match a CHECK line or (more likely) that string happens to match a CHECK-NOT line, the test will fail for that user. This leads to seemingly random test failures and causes great suffering.
> 
> This is another reason to prefer complete auto-generation of the CHECK lines using the script at 'utils/update_test_checks.py'. It doesn't generate 'NOT' lines, so there's less risk of hard-to-debug test failure.
Thanks for the comments.  I've regenerated the tests with 'utils/update_test_checks.py'.


https://reviews.llvm.org/D38232





More information about the llvm-commits mailing list