Page MenuHomePhabricator

[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.