This is an archive of the discontinued LLVM Phabricator instance.

[IR] Let IRBuilder's CreateVectorSplat/CreateShuffleVector use poison as placeholder
ClosedPublic

Authored by aqjune on Dec 23 2020, 9:02 PM.

Details

Summary

This patch updates IRBuilder to create insertelement/shufflevector using poison as a placeholder.

Diff Detail

Event Timeline

aqjune created this revision.Dec 23 2020, 9:02 PM
aqjune requested review of this revision.Dec 23 2020, 9:02 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptDec 23 2020, 9:02 PM
nlopes added inline comments.Dec 24 2020, 2:57 AM
llvm/lib/IR/IRBuilder.cpp
1019–1021

while at it, don't you want to change this one to poison as well?

aqjune added inline comments.Dec 25 2020, 9:10 AM
llvm/lib/IR/IRBuilder.cpp
1019–1021

It would be great, but there are many other places that create shufflevector with undef args (InstCombineCalls, etc). Since this and related patches are big changes, I think it might be good to land these first.

nikic added inline comments.Dec 27 2020, 12:47 PM
llvm/lib/IR/IRBuilder.cpp
1012

undef vector -> poison vector

1019–1021

I think in this case it makes sense to change both at once, as it's part of one construction. It's odd that one uses undef and the other poison, even though both are equally non-demanded.

aqjune added inline comments.Dec 27 2020, 2:00 PM
llvm/lib/IR/IRBuilder.cpp
1019–1021

If the relevant patches are to be landed at once, I think the series of patches are being too big to maintain... :(
I'd like to make shufflevector's placeholder as poison in another iteration.

aqjune added inline comments.Dec 27 2020, 11:33 PM
llvm/lib/IR/IRBuilder.cpp
1019–1021

Okay, I'll copy tests that have poison as shufflevector's placeholder value as well (will have the *-inseltpoison.ll suffix too) & update this patch.

aqjune updated this revision to Diff 313945.Dec 29 2020, 1:56 AM

Update IRBuilder to fill poison at shufflevector's empty operand

aqjune retitled this revision from [IR] Let IRBuilder's CreateVectorSplat use poison as inselt's placeholder to [IR] Let IRBuilder's CreateVectorSplat/CreateShuffleVector use poison as placeholder.Dec 29 2020, 1:57 AM
aqjune edited the summary of this revision. (Show Details)
aqjune added inline comments.
clang/test/CodeGen/SystemZ/builtins-systemz-zvector-constrained.c
84

These SystemZ zvector tests are still creating shufflevector with undef, and I couldn't find where they stem from. :/

aqjune marked 2 inline comments as done.Dec 29 2020, 1:59 AM
nikic accepted this revision.Dec 29 2020, 6:13 AM

There are some polly tests that also need to be updated, but otherwise LGTM.

This revision is now accepted and ready to land.Dec 29 2020, 6:13 AM
spatel added inline comments.Dec 29 2020, 7:24 AM
llvm/include/llvm/IR/IRBuilder.h
2527

update code comment: undefined -> poison

llvm/lib/IR/IRBuilder.cpp
1019–1021

I'm still catching up on reviews/mails, so ignore if this was already discussed:
Can we change this usage of CreateShuffleVector to the unary operand version, so we're reducing the number of explicit places where we need to specify poison?

aqjune updated this revision to Diff 313989.Dec 29 2020, 11:20 AM

Update comments
Call unary CreateShuffleVector
Update polly tests

aqjune marked an inline comment as done.Dec 29 2020, 11:21 AM
aqjune added inline comments.
llvm/lib/IR/IRBuilder.cpp
1019–1021

Yes, it makes sense.
I'll update D93817 to use the unary version as well.

This revision was landed with ongoing or failed builds.Dec 29 2020, 11:35 AM
This revision was automatically updated to reflect the committed changes.
aqjune marked an inline comment as done.
Allen added a subscriber: Allen.Jan 5 2022, 12:04 AM

I have a babyism question, why poison is preferred to the undef in the pattern ConstantVector::getSplat ?

aqjune added a comment.Jan 5 2022, 6:31 AM

I have a babyism question, why poison is preferred to the undef in the pattern ConstantVector::getSplat ?

Hi Allen,
It is because folding poison is easier than folding undef. :)
For example, according to the poison propagating rule, 'and i32 poison, 1' can be simplified into 'i32 poison'. However, 'and undef, 1' cannot be folded into 'undef' since the mask enforces all bits but LSB 0.
This patch helps simplifications happen more easily.

bmahjour added inline comments.
llvm/test/Transforms/LoopVectorize/PowerPC/optimal-epilog-vectorization.ll
273–276

these lines should not have been removed!

556–559

these lines should not have been removed

efriedma added inline comments.Jan 14 2022, 11:36 AM
llvm/test/Transforms/LoopVectorize/PowerPC/optimal-epilog-vectorization.ll
273–276

It looks like someone manually hacked on the CHECK lines in a test marked "; NOTE: Assertions have been autogenerated by utils/update_test_checks.py". This is generally a bad idea... it makes it easy to miss lines like this. If CHECKs are manually written or modified, the NOTE should be removed as a warning to anyone modifying the test in the future.

Not sure what the best solution is here... at minimum, I guess we can just drop the "autogenerated" note?

bmahjour added inline comments.Jan 14 2022, 11:52 AM
llvm/test/Transforms/LoopVectorize/PowerPC/optimal-epilog-vectorization.ll
273–276

Yes, I should have removed that comment (lesson learned), although that wouldn't necessarily have prevented the oversight.

We cannot put those lines back until the regression I reported in https://reviews.llvm.org/D116479 is fixed. I can remove the autogen note at that point.