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

[Sanjay Patel via Phabricator via llvm-commits]

spatel added inline comments.


================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:597-599
 class MergeICmps : public FunctionPass {
  public:
   static char ID;
----------------
The indentation is non-standard; you should run clang-format before this patch to cleanup.


================
Comment at: lib/Transforms/Scalar/MergeICmps.cpp:631
+  unsigned MaxLoadSize;
+  if(!TTI->enableMemCmpExpansion(MaxLoadSize)) return PreservedAnalyses::all();
+
----------------
Sorry - that comment wasn't clear. I meant there should be a space after 'if':
  if(!TTI...
should be:
  if (!TTI...


================
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
 
----------------
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.


================
Comment at: test/Transforms/MergeICmps/X86/pair-int32-int32.ll:27
   ret i1 %4
 ; CHECK-LABEL: @opeq1(
 ; The entry block with zero-offset GEPs is kept, loads are removed.
----------------
Here and in the other files, I don't think this is working as you expected. If you specify a '--check-prefix', then generic 'CHECK' is no longer used for that RUN. You should make this 'X86-LABEL' in this case.


https://reviews.llvm.org/D38232