[PATCH] D93818: [LangRef] Update shufflevector's semantics to return poison if the mask is undef

[Juneyoung Lee via Phabricator via llvm-commits]

aqjune added a comment.

In D93818#2805725 <https://reviews.llvm.org/D93818#2805725>, @spatel wrote:

> In D93818#2804522 <https://reviews.llvm.org/D93818#2804522>, @aqjune wrote:
>
>> Hi all,
>>
>> Before diving into further updates in InstCombine (D94380 <https://reviews.llvm.org/D94380> and D93817 <https://reviews.llvm.org/D93817>), what about pinning down this LangRef update first & letting people know via llvm-dev mail?
>
> That sounds good to me. I'm not sure about the different pieces that are left. I know we already updated many, but not all, places in IR to use poison *vector operand* for shufflevector with the change to IRBuilder, so making things consistent for that seems like an obviously good step. Changing the shuffle *mask* has different concerns? If there is still regression potential, can you re-describe those (here or on llvm-dev)?

The left pieces are mainly about transformations that should preserve undefinedness of the don't-care vector. Consider this optimization

  (InstCombineCast's shrinkSplatShuffle)
      // trunc (shuf X, Undef, SplatMask) --> shuf (trunc X), Undef, SplatMask

Since shufflevector's placeholder value is now poison, we can naively fix to this:

  // trunc (shuf X, poison, SplatMask) --> shuf (trunc X), poison, SplatMask

But it means this transformation won't fire if the placeholder value was (accidentally) undef.
Avoiding this regression requires patches to contain more than simple syntactic UndefValue->PoisonValue replacement.

The difference between poison *vector operand* and poison *shuffle mask*: yep, they are different things, but related as well.
The whole story was as follows:
(1) To fix the bugs, it was concluded that insertelement's operand and shuffle mask must be poison.
(2) It was suggested that shufflevector's vector operand should be poison as well, since many transformations simply reuse insertelement's don't-care vector to create shufflevector with don't-care vector operand.
(3) A series of patches were made to fix insertelement/shufflevector's don't-care vector operand to be poison.
This LangRef is the first patch to make shuffle mask poison.

IMO there is no regression potential in theory. As select->and/or was done well, if transformations are properly updated things will work fine.

>> The remaining updates at D94380 <https://reviews.llvm.org/D94380> and D93817 <https://reviews.llvm.org/D93817> are about non-syntactic changes; before them, many syntactic updates to make the placeholder `poison` have been done so far (D9381 <https://reviews.llvm.org/D9381> has links to such patches).
>
> Lost a digit in that link (D9381 <https://reviews.llvm.org/D9381>?).

Oops, it was D93817 <https://reviews.llvm.org/D93817>, sorry.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D93818/new/

https://reviews.llvm.org/D93818