This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Fix miscompile bug in canEvaluateShuffled
ClosedPublic

Authored by bjope on Oct 16 2019, 7:11 AM.

Details

Summary

Add restrictions in canEvaluateShuffled to prevent that we for example
transform

%0 = insertelement <2 x i16> undef, i16 %a, i32 0
%1 = srem <2 x i16> %0, <i16 2, i16 1>
%2 = shufflevector <2 x i16> %1, <2 x i16> undef, <2 x i32> <i32 undef, i32 0>

into

%1 = insertelement <2 x i16> undef, i16 %a, i32 1
%2 = srem <2 x i16> %1, <i16 undef, i16 2>

as having an undef denominator makes the srem undefined (for all
vector elements).

Fixes: https://bugs.llvm.org/show_bug.cgi?id=43689

Diff Detail

Event Timeline

bjope created this revision.Oct 16 2019, 7:11 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 16 2019, 7:11 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
bjope marked an inline comment as done.Oct 16 2019, 7:13 AM
bjope added inline comments.
llvm/test/Transforms/InstCombine/shufflevector-div-rem.ll
4

Not sure if I need to pre-commit the test case (?).
If so, then maybe these comments describing the test cases should be updated to tell that it shows the bad result in the pre-commit.

lebedev.ri marked an inline comment as done.Oct 16 2019, 7:26 AM
lebedev.ri added a subscriber: lebedev.ri.
lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
1069

Does this apply to FP too? (i'm not seeing that in langref)
If it does, then the comment needs updating.

1071

This -1 always looks so weird to me.
It would be really good to cleanup it everywhere one day to proper SHUFFLE_UNDEF_INDEX or something.

llvm/test/Transforms/InstCombine/shufflevector-div-rem.ll
4

This shows diff so it's okay in principle.
Tests for other changed opcodes?

spatel added inline comments.Oct 16 2019, 7:49 AM
llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
1069

FDiv (or any other non-constrained FP opcode) is safe; it should not be included in this patch. Might want to add a negative test case to confirm that and make sure nobody makes that mistake going forward.

bjope added inline comments.Oct 16 2019, 8:46 AM
llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
1069

Oopsy-daisy, good thing there are reviewers. FDiv was actually included by mistake here.

I will duplicate the "test2" test case for udiv/sdiv/urem/fdiv/frem, making the tests for fdiv/frem "negative" (expecting that the transform is done)

bjope updated this revision to Diff 225257.Oct 16 2019, 10:36 AM
  • Fixup mistake regarding FDiv (it was not supposed to be included in the new restriction).
  • Added test cases for sdiv/urem/udiv (showing that the new restriction works).
  • Added test cases for fdiv/frem (showing that we can eliminate the shufflevector).
bjope marked 4 inline comments as done.Oct 16 2019, 10:37 AM
lebedev.ri accepted this revision.Oct 16 2019, 10:50 AM

Please do make sure to precommit tests.
LGTM, but please wait for @spatel.

llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
1069–1071
if(llvm::any_of(Mask, [](int M){ return M == -1; }))
  return false;
1100–1101

Is this comment now outdated?

This revision is now accepted and ready to land.Oct 16 2019, 10:50 AM
spatel accepted this revision.Oct 16 2019, 11:25 AM

LGTM with minor fixes as suggested.

llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
1068

This comment could provide a better explanation. Something like:

// Propagating an undefined shuffle mask element to integer
// div/rem is not allowed because those opcodes can create
// immediate undefined behavior from an undefined element
// in an operand.
This revision was automatically updated to reflect the committed changes.
bjope marked 2 inline comments as done.Oct 18 2019, 12:57 AM