This is an archive of the discontinued LLVM Phabricator instance.

[MergeICmps] Fix missing split.
ClosedPublic

Authored by courbet on Dec 1 2020, 2:32 AM.

Details

Summary

We were not correctly splitting a blocks for chains of length 1.

This supersedes D92364.

Test case by MaskRay (Fangrui Song).

Diff Detail

Event Timeline

courbet created this revision.Dec 1 2020, 2:32 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 1 2020, 2:32 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
courbet requested review of this revision.Dec 1 2020, 2:32 AM
jyknight added a subscriber: jyknight.
jyknight added inline comments.
llvm/test/Transforms/MergeICmps/X86/gep-references-bb.ll
6–8

Indirection unnecessary; can just put i32 directly in %Triple

10–12

Comment needs to be fixed.

13

This example should've really been transformed into a single memcmp of 12 bytes, not a cmp of the first 4-bytes, and a memcmp of the latter 8. So, there's a missed-optimization bug in this pass, as well. (I guess it's only looking at the direct pointer arg of the GEP, not following through them the chain of GEPs to find the base object.)

That missed-opt shouldn't be fixed at the same time as the miscompile, of course, but the current test is fragile w.r.t. that missing optimization -- if we fix the pass to compile this to a single 12-byte memcmp, we'll no longer have the test case for a size==1 comparison.

I think it would be good to make the test insensitive to that opt by adding another i32 field in the struct, between the first and the second, which gets ignored by the compares -- and update the size to dereferenceable(16). That'll still trigger the misoptimization, but not be transformable into a single memcmp, so this test will still test what happens for the chain of length 1 case -- even after fixing that issue in a future change.

Maybe also include a side-affecting instruction at the top of the function which should _definitely_ not be removed, like store i32 1, i32* @global.

courbet updated this revision to Diff 308634.Dec 1 2020, 6:23 AM

Update comment.

courbet updated this revision to Diff 308641.Dec 1 2020, 6:40 AM

Simplify the test as suggested by James.

courbet updated this revision to Diff 308642.Dec 1 2020, 6:43 AM

Make the test more robust to future optimizations as suggested by James.

courbet updated this revision to Diff 308645.Dec 1 2020, 6:49 AM

...and add a side-effecting instruction at the top.

courbet marked 2 inline comments as done.Dec 1 2020, 6:50 AM

Thanks for the comments.

jyknight accepted this revision.Dec 1 2020, 7:01 AM
This revision is now accepted and ready to land.Dec 1 2020, 7:01 AM
Harbormaster completed remote builds in B80658: Diff 308641.

Oh -- if it's not too late, can you fix the commit message before pushing -- it could probably do with a little more explanation.

courbet updated this revision to Diff 308658.Dec 1 2020, 7:43 AM

Improve commit message.

This revision was landed with ongoing or failed builds.Dec 1 2020, 7:51 AM
This revision was automatically updated to reflect the committed changes.
MaskRay added a comment.EditedDec 1 2020, 9:09 AM

As a follow-up, would be good adding some tests where the pointer operand of a GEP references an instruction defined in a basic block. Currently in all tests the pointer operands refer to arguments.