This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2)
AbandonedPublic

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

Details

Summary

This patch updates the remaining transformations to create insertelement/shufflevector with poison placeholder.

Diff Detail

Event Timeline

aqjune created this revision.Dec 25 2020, 3:14 PM
aqjune requested review of this revision.Dec 25 2020, 3:14 PM
Herald added a project: Restricted Project. · View Herald TranscriptDec 25 2020, 3:14 PM
spatel added inline comments.Dec 29 2020, 7:53 AM
llvm/lib/CodeGen/CodeGenPrepare.cpp
6726–6727 ↗(On Diff #313736)

Similar to my comment in D93793 - if we convert this (and any others like this) into the unary CreateShuffleVector call, we can reduce the number of code diffs and chances that something is escaping the transition from undef to poison.

aqjune updated this revision to Diff 314123.Dec 30 2020, 8:40 AM

Rebase, update transformations with shufflevector involved too

Herald added a project: Restricted Project. · View Herald TranscriptDec 30 2020, 8:40 AM
aqjune retitled this revision from Update transformations to use poison for new insertelement's placeholder value to Update transformations to use poison for insertelement/shufflevector's placeholder value.Dec 30 2020, 8:43 AM
aqjune edited the summary of this revision. (Show Details)

I'll share the result after checking the validity of this patch using alive2.

aqjune updated this revision to Diff 314175.Dec 31 2020, 1:03 AM

Find a few missing places & update them to use poison

aqjune added a comment.EditedDec 31 2020, 1:14 AM

Alive2 result: Running Alive2 finds one failure, which is fixed by updating the shufflevector semantics to return poison on undef/poison mask (D93818):

Transforms/InstCombine/shufflevector-div-rem-inseltpoison.ll

----------------------------------------                                           
define <2 x float> @test_fdiv(float %a, float %b, i1 %cmp) {                       
%0:                                                                                
  %splatinsert = insertelement <2 x float> poison, float %a, i32 0                 
  %denom = insertelement <2 x float> { 3.000000, poison }, float 1.000000, i32 1   
  %t1 = fdiv <2 x float> %splatinsert, %denom                                      
  %splat.op = shufflevector <2 x float> %t1, <2 x float> poison, 4294967295, 0     ; THIS BLOCKS POISON
  %t2 = select i1 %cmp, <2 x float> { 77.000000, 99.000000 }, <2 x float> %splat.op
  ret <2 x float> %t2                                                              
}                                                                                  
=>                                                                                 
define <2 x float> @test_fdiv(float %a, float %b, i1 %cmp) {                       
%0:                                                                                
  %1 = insertelement <2 x float> poison, float %a, i32 1                           
  %2 = fdiv <2 x float> %1, { undef, 3.000000 }                                    
  %t2 = select i1 %cmp, <2 x float> { 77.000000, 99.000000 }, <2 x float> %2       
  ret <2 x float> %t2                                                              
}                                                                                  
Transformation doesn't verify!                                                     
ERROR: Target is more poisonous than source

Diffs in llvm/test and clang/test: I checked that all of these are replacements of undef with poison, and there was no larger change.

Remaining undef placeholder: After this patch, there are two places where undef still shows up as a placeholder from llvm tests: I couldn't find where they are coming from. :(
If someone points me where they're likely to appear, I'll fix these.

$ ag "shufflevector <.*> .*, <.*> undef" | grep inseltpoison
Transforms/InstCombine/X86/x86-pshufb-inseltpoison.ll

$ ag "insertelement <.*> undef" | grep inseltpoison
Transforms/InstCombine/AMDGPU/amdgcn-demanded-vector-elts-inseltpoison.ll
aqjune added a comment.Jan 2 2021, 6:49 PM

I'll split this patch into smaller pieces, so they're able to get reviewed more easily.

aqjune updated this revision to Diff 314289.EditedJan 3 2021, 8:43 AM

Factored out non-instcombine diffs; I'll split the instcombine diffs into several patches too

aqjune updated this revision to Diff 315691.Jan 10 2021, 6:55 PM

Leave updates in InstCombineVectorOps.cpp only

aqjune retitled this revision from Update transformations to use poison for insertelement/shufflevector's placeholder value to [InstCombine] Update transformations to use poison for insertelement/shufflevector's placeholder value (2/2).Jan 10 2021, 6:57 PM
aqjune edited the summary of this revision. (Show Details)
aqjune added inline comments.Jan 10 2021, 7:18 PM
llvm/lib/Transforms/InstCombine/InstCombineSimplifyDemanded.cpp
1221

This change is okay because it is splat (only the first vector of shufflevector was accessed)

llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
551
602

Okay to be poison (the added comment above replaceExtractElements has details)

1132

Since this is creating a splat, this shufflevector change and the insertelement change above is okay.

1344

Values is the RHS of a new shufflevector below.
The poison elements are unused elems, so okay to be poison.

1441

When LR.second was nullptr, it was simply a placeholder (so okay to be poison)

2355

As seen in line 2353, Elts' indices are smaller than LHSWidth, so okay to be poison.

Matt added a subscriber: Matt.Jun 8 2021, 3:43 AM
aqjune abandoned this revision.Sep 25 2021, 8:12 PM

I abandon this patch because (1) shufflevector parts are covered in D110226, D110227, D110230, and (2) insertelement parts will be covered in upcoming new patches.