This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] Change SimplifyDemandedVectorElts to use PoisonElts instead of UndefElts
Changes PlannedPublic

Authored by ManuelJBrito on Jun 11 2023, 2:35 PM.

Details

Summary

This patch changes InstCombine's SimplifyDemandedVectorElts to use PoisonElts instead of UndefElts. It now treats undef elements as DemandedElts and keeps track of poison elements only.
This fixes some undef related bugs marked with LLVM PR44185 in

https://web.ist.utl.pt/nuno.lopes/alive2/index.php?hash=a9bc405e427c007f.

Such as:

Transforms/InstCombine/insert-const-shuf.ll
Transforms/InstCombine/sub-of-negatible.ll
Transforms/InstCombine/vec_shuffle.ll

At this point, frontends should not be emitting undef values, so losing the undef optimizations should be OK.

In most cases, it should be OK to go from UndefElts to PoisonElts because PoisonElts is a subset of UndefElts.
The only cases that might be of concern are the ones where target specific intrinsics are optimized based on SimplifyDemandedVectorElts.
After this patch they might be emitting poison where there was previously undef. This might break the semantics of those intrinsics. Requesting reviewers for those cases.

This change also allows for more aggressive optimization because poison is more undefined than undef. ​For example:

gep(X, poison) -> poison is OK 
gep(X, undef) -> undef is NOT OK. (see https://github.com/llvm/llvm-project/issues/44790)

Previously we weren't allowed to do this because there was no way to distinguish between undef and poison elements.
These optimizations can be done in follow-up patches.

Diff Detail

Event Timeline

ManuelJBrito created this revision.Jun 11 2023, 2:35 PM
Herald added a project: Restricted Project. · View Herald Transcript
Herald added subscribers: pmatos, asb, foad and 7 others. · View Herald Transcript
ManuelJBrito requested review of this revision.Jun 11 2023, 2:35 PM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJun 11 2023, 2:35 PM
nlopes added inline comments.Jun 11 2023, 3:08 PM
llvm/lib/Target/X86/X86InstCombineIntrinsic.cpp
3102–3105

this change doesn't look correct. The bits are undefined, doesn't seem like you can use poison here.

llvm/test/Transforms/InstCombine/vec_shuffle.ll
1302

Why does it regress here (goes from poison to undef)?

ManuelJBrito added inline comments.Jun 16 2023, 8:45 AM
llvm/lib/Target/X86/X86InstCombineIntrinsic.cpp
3102–3105

Thanks, I'll fix it. This will cause some regressions in the cases where only the upper half is demanded.
Eventually we could fold those cases to freeze(poison).

llvm/test/Transforms/InstCombine/vec_shuffle.ll
1302

This regression is due to the change in foldVectorBinop in InstructionCombining.cpp
It seems that without this canonicalization, demanded elements doesn't trigger causing the regression.

I changed it to match poison instead of undef, because of the following:

shufflevector < 4 x i32 > %1, < 4 x i32 > undef , <1, 2, 6, 7> --> shufflevector < 4 x i32 > %1, < 4 x i32 > undef , <1, 2, undef, undef>

Now we don't do this because it would insert poison.

Not simplifying the mask causes the following assert to fail:

assert(ShMask[I] < (int)NumElts && "Not expecting narrowing shuffle")

Address comments.
Cannot use poison for undefined part of result in SSE4A intrinsics.
Regressions:

  • x86-sse4a-inseltpoison.ll
  • x86-sse4a.ll

Fix regressions in vec_shuffle.ll by skipping mask elements from second operand in foldVectorBinop.

rebase & ping

Hi, thanks for your hard work!

llvm/lib/Transforms/InstCombine/InstructionCombining.cpp
1653

This seems to be

1653

Nevermind, please ignore this comment.

llvm/test/Transforms/InstCombine/vec_shuffle.ll
373

Do you have any idea about the reason of this regression?

llvm/test/Transforms/InstCombine/vector-casts-inseltpoison.ll
297

This looks interesting - do you have idea how the undef elements are introduced?

llvm/test/Transforms/SLPVectorizer/X86/alternate-int.ll
451

Do you have any idea about why the 5'th element is undef, not poison?

llvm/test/Transforms/SLPVectorizer/X86/pr49081.ll
11

This change looks interesting, I think it is worth investigating why this happened.

ManuelJBrito planned changes to this revision.Jul 18 2023, 3:09 AM

Hello @aqjune thanks for the comments !! Some users of SimplifyDemandedVectorElts might need some more tweaking to prevent regressions for the cases you noted. I'll work on this as soon as I can!