This is an archive of the discontinued LLVM Phabricator instance.

[InstCombine] remove useless insertelement
ClosedPublic

Authored by Chenbing.Zheng on Jun 30 2022, 1:27 AM.

Details

Summary

Remove insertelement, if we don't use the inserted element.
extractelement (bitcast (insertelement (Vec, b)), a) -> extractelement (bitcast (Vec), a)

Diff Detail

Event Timeline

Chenbing.Zheng created this revision.Jun 30 2022, 1:27 AM
Herald added a project: Restricted Project. · View Herald TranscriptJun 30 2022, 1:27 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
Chenbing.Zheng requested review of this revision.Jun 30 2022, 1:27 AM

Shouldn't this be in InstCombinerImpl::SimplifyDemandedVectorElts ?

Shouldn't this be in InstCombinerImpl::SimplifyDemandedVectorElts ?

'SimplifyDemandedVectorElts' cannot support scalable vector type now, and it seems to be no need to break this setting.

In which case, we need a FIXME comment explaining that this should be removed once scale vectors are supported

In which case, we need a FIXME comment explaining that this should be removed once scale vectors are supported

In D78895, @huihuiz add this limit for SimplifyDemandedVectorElts. Maybe SimplifyDemandedVectorElts will no longer support scale vectors.

RKSimon added a subscriber: dmgreen.Jul 4 2022, 2:28 AM

In which case, we need a FIXME comment explaining that this should be removed once scale vectors are supported

In D78895, @huihuiz add this limit for SimplifyDemandedVectorElts. Maybe SimplifyDemandedVectorElts will no longer support scale vectors.

More likely its untested / has edge cases and they haven't the bandwidth to address it yet. @dmgreen is currently dealing with a similar limit that was added in DAG (D128918)

Adding code to foldBitcastExtElt to get around this limitation must be seen as a temporary measure only.

add fixme comment

RKSimon added inline comments.Jul 5 2022, 1:27 AM
llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
250

Are there tests for multiuse?

llvm/test/Transforms/InstCombine/vscale_extractelement-inseltpoison.ll
44

technically its not a wrong insert - 'useless' might be a better term!

llvm/test/Transforms/InstCombine/vscale_extractelement.ll
44

technically its not a wrong insert - 'useless' might be a better term!

complete tests

RKSimon accepted this revision.Jul 6 2022, 1:06 AM

LGTM - but please add back the FIXME that you're lost in the rebase

llvm/lib/Transforms/InstCombine/InstCombineVectorOps.cpp
248

Add back the FIXME

This revision is now accepted and ready to land.Jul 6 2022, 1:06 AM
This revision was landed with ongoing or failed builds.Jul 6 2022, 2:05 AM
This revision was automatically updated to reflect the committed changes.