This is an archive of the discontinued LLVM Phabricator instance.

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

Authored by courbet on Sep 25 2017, 4:15 AM.

Details

Summary

This is to avoid e.g. merging two cheap icmps if the target is not going to expand to something nice later.

Diff Detail

Repository
rL LLVM

Event Timeline

courbet created this revision.Sep 25 2017, 4:15 AM
spatel edited edge metadata.Oct 3 2017, 2:32 PM
spatel added a subscriber: llvm-commits.

Adding llvm-commits to subscribers.

spatel added inline comments.Oct 3 2017, 2:40 PM
lib/Transforms/Scalar/MergeICmps.cpp
46 ↗(On Diff #116514)

Remove rather than comment out?

633 ↗(On Diff #116514)

formatting - more spacious.

test/Transforms/MergeICmps/pair-int32-int32.ll
1 ↗(On Diff #116514)

I suspect (because I've done it a few times myself!) that these tests are going to fail on bots that don't build the x86 target. I think you'll need to create target-specific subdirectories for these tests and split them up into target-independent and target-required RUNs.

You can probably use the test structure under tests/Transforms/SimplifyCFG as a model for how it needs to be done.

courbet updated this revision to Diff 117644.Oct 4 2017, 2:43 AM
courbet marked 3 inline comments as done.
  • cosmetics
  • split the tests into x86 and non-x86
courbet updated this revision to Diff 117645.Oct 4 2017, 2:46 AM
This comment was removed by courbet.
courbet updated this revision to Diff 117647.Oct 4 2017, 2:50 AM
  • cosmetics
  • split the tests into x86 and non-x86

Thanks, PTAL.

test/Transforms/MergeICmps/pair-int32-int32.ll
1 ↗(On Diff #116514)

Thanks for the pointer !

spatel added inline comments.Oct 4 2017, 7:17 AM
lib/Transforms/Scalar/MergeICmps.cpp
597–599 ↗(On Diff #117647)

The indentation is non-standard; you should run clang-format before this patch to cleanup.

631 ↗(On Diff #117647)

Sorry - that comment wasn't clear. I meant there should be a space after 'if':

if(!TTI...

should be:

if (!TTI...
test/Transforms/MergeICmps/X86/pair-int32-int32.ll
1 ↗(On Diff #117647)

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.

27 ↗(On Diff #117647)

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.

courbet updated this revision to Diff 117675.Oct 4 2017, 8:17 AM

Rebase on clang-formatted MergeIcmps.cpp

courbet updated this revision to Diff 117683.Oct 4 2017, 9:21 AM
courbet marked 3 inline comments as done.
  • use update_test_checks.py to generate tests.

Thanks, PTAL.

lib/Transforms/Scalar/MergeICmps.cpp
597–599 ↗(On Diff #117647)

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 ?

test/Transforms/MergeICmps/X86/pair-int32-int32.ll
1 ↗(On Diff #117647)

Thanks for the comments. I've regenerated the tests with 'utils/update_test_checks.py'.

spatel accepted this revision.Oct 4 2017, 9:31 AM

LGTM.

lib/Transforms/Scalar/MergeICmps.cpp
597–599 ↗(On Diff #117647)

Strange - when I run clang-format locally, it puts the 'public' at the left edge (no indent). Oh well, it was just a nit. :)

This revision is now accepted and ready to land.Oct 4 2017, 9:31 AM

Thanks for the review.

This revision was automatically updated to reflect the committed changes.
llvm/trunk/test/Transforms/MergeICmps/pair-int32-int32.ll