This is an archive of the discontinued LLVM Phabricator instance.

[X86] Fix bug in r288804.
AbandonedPublic

Authored by aymanmus on Dec 8 2016, 7:29 AM.

Details

Summary

When creating the load from constant pool, check if there was already a load to the same constant, and abort if found.
The additional use of the load will prevent the pattern from memory folding the broadcast.

Diff Detail

Event Timeline

aymanmus updated this revision to Diff 80760.Dec 8 2016, 7:29 AM
aymanmus retitled this revision from to [X86] Fix bug in r288804..
aymanmus updated this object.
aymanmus added reviewers: mkuper, RKSimon, spatel, zvi.
aymanmus added a subscriber: llvm-commits.
zvi edited edge metadata.Dec 8 2016, 8:07 AM

Please add a test

aymanmus updated this revision to Diff 80766.Dec 8 2016, 9:13 AM
aymanmus edited edge metadata.

+Test

mkuper edited edge metadata.Dec 8 2016, 9:55 AM

A drive-by comment - regardless of whether we should be forming a vbroadcast for this case (I'll let you and Zvi figure it out :-) ), shouldn't we also have a pattern in place for the case we do?

X86InstrAVX512.td has a lot of X86SubVBroadcast patterns that are commented with:
Provide fallback in case the load node that is used in the patterns above
is used by additional users, which prevents the pattern selection.
But I don't see such a pattern for the v4i64 -> v8i64 case.

andreadb added inline comments.Dec 8 2016, 11:29 AM
lib/Target/X86/X86ISelLowering.cpp
6414–6415

X86ISD::VBROADCAST requires a memory operand only if the target is not AVX2.
On AVX2, we should still be able to select a register-register variant of VBROADCAST from a X86ISD::VBROADCAST.
Basically (at least on AVX2) I don't think that a X86ISD::VBROADCAST is problematic even if we have load with multiple uses.

6435–6436

Would it be possible to check in advance if the Ld node is already used?
In case, we would know in advance that the check at line 6455 would fail. That would let us early exit before we even create a node for the broadcast.

6445–6449

Same as above. Early exit if we know in advance that Ld has already uses.

6450–6455

I think it is safe to remove the else part.
You can always check if Brdcst is null at line 6455.

I think you can write something like this:

if (Brdcst && !Ld.hasOneUse())
6453–6456

Phabricator doesn't let me delete this comment...

zvi added a comment.Dec 8 2016, 12:08 PM

Ayman, I think that @mkuper 's observation is the way to fix the PR you are addressing, followed by patches for the optimization in this patch, and @andreadb's suggestions.

aymanmus abandoned this revision.Dec 11 2016, 5:11 AM

A new review with the solution - offered by @mkuper - is up for review:
https://reviews.llvm.org/D27661