Page MenuHomePhabricator

[LangRef] Update shufflevector's semantics to return poison if the mask is undef
Needs ReviewPublic

Authored by aqjune on Dec 25 2020, 3:30 PM.

Details

Summary

This patch updates shufflevector's semantics to return poison if the mask is undef.
It resolves shufflevector-related bugs which is described at https://reviews.llvm.org/D93586.

This resolves many bugs that are marked as 'LLVM PR44185' at https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=3c479a0f6691091c .

*But*, it also makes a few transformations invalid:

Transforms/InstCombine/broadcast.ll: https://bugs.llvm.org/show_bug.cgi?id=43958
Transforms/VectorCombine/X86/load.ll: a transformation that's similar to the previous one
Transforms/InstCombine/sub-of-negatible.ll:

define <2 x i4> @negate_shufflevector_oneinput_second_lane_is_undef(<2 x i4> %x, <2 x i4> %y) {
%0:
  %t0 = shl <2 x i4> { 10, 5 }, %x
  ; t1[2] is undef, not poison
  %t1 = shufflevector <2 x i4> %t0, <2 x i4> undef, 0, 2  
  %t2 = sub <2 x i4> %y, %t1
  ret <2 x i4> %t2
}
=>
define <2 x i4> @negate_shufflevector_oneinput_second_lane_is_undef(<2 x i4> %x, <2 x i4> %y) {
%0:
  %t0.neg = shl <2 x i4> { 6, 11 }, %x
  ; t1[2] is poison
  %t1.neg = shufflevector <2 x i4> %t0.neg, <2 x i4> undef, 0, 4294967295 
  %t2 = add <2 x i4> %t1.neg, %y
  ret <2 x i4> %t2
}
Transformation doesn't verify!
ERROR: Target is more poisonous than source

Transforms/InstCombine/insert-const-shuf.ll:

define <5 x i8> @longerMask(<3 x i8> %x) {
%0:
  ; %shuf[3] = undef
  %shuf = shufflevector <3 x i8> %x, <3 x i8> { undef, 1, 2 }, 2, 1, 4, 3, 0
  %ins = insertelement <5 x i8> %shuf, i8 42, i17 4
  ret <5 x i8> %ins
}
=>
define <5 x i8> @longerMask(<3 x i8> %x) {
%0:
  ; %shuf[3] = poison
  %shuf = shufflevector <3 x i8> %x, <3 x i8> { poison, 1, poison }, 2, 1, 4, 4294967295, 4294967295
  %ins = insertelement <5 x i8> %shuf, i8 42, i17 4
  ret <5 x i8> %ins
}
Transformation doesn't verify!
ERROR: Target is more poisonous than source

I think these transformations are all solvable: we can simply disable these transformations if insertelement/shufflevector's placeholder isn't poison.

Diff Detail

Event Timeline

aqjune created this revision.Dec 25 2020, 3:30 PM
aqjune requested review of this revision.Dec 25 2020, 3:30 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 25 2020, 3:30 PM

Using undef in the shuffle mask to indicate a poison result seems pretty confusing to me. I think we should use poison as the marker instead. We could allow both undef/poison in input IR, but at least the output IR rendering should use poison (the internal representation is something else anyway, as shuffle masks are no longer Values).

Using undef in the shuffle mask to indicate a poison result seems pretty confusing to me. I think we should use poison as the marker instead. We could allow both undef/poison in input IR, but at least the output IR rendering should use poison (the internal representation is something else anyway, as shuffle masks are no longer Values).

+1, I'll make a patch for this as well.

aqjune added a comment.Jan 2 2021, 4:51 PM

It seems IR is already accepting poison masks: https://godbolt.org/z/K8ca84
Printing seems to be the only part to update. :)