This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Add support for 256-bit non temporal loads
ClosedPublic

Authored by zjaffal on Aug 12 2022, 6:34 AM.

Details

Summary

Currenlty all temporal loads are mapped to LDP or LDR. This patch will map all the non temporal 256-bit loads into LDNP. Future patches should address other non-temporal loads.

Diff Detail

Event Timeline

zjaffal created this revision.Aug 12 2022, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 6:34 AM
zjaffal requested review of this revision.Aug 12 2022, 6:34 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 12 2022, 6:34 AM
fhahn added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
795

Could you add a comment similar to the one for the stores above?

806

nit: please add a newline before this comment.

20423

Would be good to have a comment here to describe what's lowered here.

20438

leftover commented-out code?

llvm/lib/Target/AArch64/AArch64InstrInfo.td
2588

nit: newline here

dmgreen added inline comments.Aug 13 2022, 3:25 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
20424

Why is EC.isKnownEven needed?

20425

This looks like it could drop a set of brackets.

zjaffal updated this revision to Diff 452422.Aug 13 2022, 7:07 AM
zjaffal marked an inline comment as done.

Remove even checks and add comments

zjaffal marked 5 inline comments as done.Aug 16 2022, 12:47 AM
zjaffal added inline comments.
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
20424

I followed the same code that is used to lower STNP but I think it is not really necessary here.
I think we can remove it.

dmgreen accepted this revision.Aug 16 2022, 1:44 AM

LGTM if @fhahn doesn't know of a reason to add the isKnownEven.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
20424

Yeah - I agree so long as it is checking the size of the MemVT and it's scalar element size.

This revision is now accepted and ready to land.Aug 16 2022, 1:44 AM
fhahn added inline comments.Aug 16 2022, 2:16 AM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
20418–20419

I think this assertion doesn't really make sense any longer, because we now enable the custom lowering for additional types. This probably needs to became just a check (i.e add an early exit here if the cownciditon doesn't hold). At the moment this assertion triggers in multiple tests, causing them to fail.

fhahn added a comment.Aug 16 2022, 2:23 AM

LGTM if @fhahn doesn't know of a reason to add the isKnownEven.

I can't think of any reason why this should be needed now and also couldn't find anything when looking at the original review. @zjaffal it would be great if you could post a follow-up patch that removes isKnownEven from the checks for STNP or even better move the checks to a helper function used by both STNP and LDNP handling code.

zjaffal updated this revision to Diff 452947.Aug 16 2022, 4:11 AM
zjaffal marked an inline comment as done.

Fix for failing tests

fhahn accepted this revision.Aug 16 2022, 4:14 AM

LGTM, thanks!

zjaffal marked 2 inline comments as done.Aug 16 2022, 4:15 AM
This revision was landed with ongoing or failed builds.Aug 16 2022, 4:19 AM
This revision was automatically updated to reflect the committed changes.