Page MenuHomePhabricator

[EarlyCSE] Handle masked loads and stores
ClosedPublic

Authored by kparzysz on Sep 8 2020, 5:57 PM.

Details

Summary

Extend the handling of memory intrinsics to also include non-target-specific intrinsics, in particular masked loads and stores.

Invent isHandledNonTargetIntrinsic to distinguish between intrinsics that should be handled natively from intrinsics that can be passed to TTI.

Add code that handles masked loads and stores and update the testcase to reflect the results.

Diff Detail

Event Timeline

kparzysz created this revision.Sep 8 2020, 5:57 PM
Herald added a project: Restricted Project. · View Herald TranscriptSep 8 2020, 5:57 PM
Herald added subscribers: jfb, hiraditya. · View Herald Transcript
kparzysz requested review of this revision.Sep 8 2020, 5:57 PM

Ping?

This should be NFC for everything that doesn't involve masked loads/stores.

This should be NFC for everything that doesn't involve masked loads/stores.

Can us split that part out then? Makes reviews easier.

Created D87691 with the refactoring. Will rebase this patch soon.

kparzysz updated this revision to Diff 291937.Sep 15 2020, 8:08 AM

Rebased on top of D87691.

Overall this seems like a natural extension of the existing EarlyCSE handling of load/store. I don't see any major issues.

llvm/lib/Transforms/Scalar/EarlyCSE.cpp
701

Info.MatchingID is getting implicitly set to zero? Could that conflict with a target-specific ID? I guess you explicitly check the identity of the intrinsic later, but it seems a little confusing that you aren't doing anything explicit with getMatchingId().

873

Is there any reason to expect !isa<UndefValue>(Mask0) to hold here? I mean, it would be weird to see a load with an undef mask, but I don't see any code that would enforce it, and the assertion itself doesn't explain.

876

The Vec0->getType() != Vec1->getType() shouldn't be necessary: if the values are the same type, that implies the masks are the same type.

884

There should be some test coverage for the unequal-masks case.

If the unequal-masks cases are important, you might run into cases where a masked operation with an all-true mask got folded to a plain load/store. Not sure how likely that is.

kparzysz marked 4 inline comments as done.Sep 18 2020, 12:11 PM
kparzysz added inline comments.
llvm/lib/Transforms/Scalar/EarlyCSE.cpp
701

Setting it to masked_load (with an explanation in a comment).

873

You're right, I changed it to an explicit check.

876

There is a check if the pointer operands are equal, so you're right, the types here are implied to be the same. I added it as a safeguard for when that check (which is at another place) is relaxed to checking that the pointers point to the same location, but are not necessarily equal as Value's. I changed the type check here to an assertion, so this will still fail when this condition is accidentally violated.

kparzysz updated this revision to Diff 292866.Sep 18 2020, 12:12 PM
kparzysz marked 3 inline comments as done.

Add testcases with unequal masks.

Fix detecting dead masked stores.

lebedev.ri added inline comments.
llvm/test/Transforms/EarlyCSE/masked-intrinsics-unequal-masks.ll
1

Precommit new tests please

kparzysz updated this revision to Diff 292879.Sep 18 2020, 12:54 PM

Pre-committed new test file.

This revision is now accepted and ready to land.Sep 21 2020, 1:57 PM
efriedma requested changes to this revision.Sep 21 2020, 2:00 PM
efriedma added inline comments.
llvm/lib/Transforms/Scalar/EarlyCSE.cpp
883

Just spotted an issue here.

This cast<ConstantInt> could fail. The operand of a ConstantVector has to be a Constant, but it doesn't have to be a ConstantInt. It could be an UndefValue, or a ConstantExpr.

This revision now requires changes to proceed.Sep 21 2020, 2:00 PM
kparzysz updated this revision to Diff 293255.Sep 21 2020, 2:31 PM

Fixed submask detection.

kparzysz marked an inline comment as done.Sep 21 2020, 2:32 PM
efriedma added inline comments.Sep 21 2020, 4:32 PM
llvm/lib/Transforms/Scalar/EarlyCSE.cpp
898

!Int0->isZero() && Int1->isZero() is always true.

kparzysz updated this revision to Diff 293287.Sep 21 2020, 4:40 PM

Removed unnecessary checks in isSubmask.

kparzysz marked an inline comment as done.Sep 21 2020, 4:40 PM
This revision is now accepted and ready to land.Sep 21 2020, 4:43 PM
This revision was landed with ongoing or failed builds.Sep 21 2020, 4:47 PM
This revision was automatically updated to reflect the committed changes.