This is an archive of the discontinued LLVM Phabricator instance.

[AArch64] Fix arm neon vstx lane memVT size
ClosedPublic

Authored by hstk30 on Aug 23 2023, 6:30 AM.

Details

Summary

StN lane memory size set too big lead to alias analysis goes wrong.
https://github.com/llvm/llvm-project/issues/64696

Diff Detail

Event Timeline

hstk30 created this revision.Aug 23 2023, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 6:30 AM
hstk30 requested review of this revision.Aug 23 2023, 6:30 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 23 2023, 6:30 AM
hstk30 retitled this revision from fix arm neon vstx lane memVT size to [AArch64] Fix arm neon vstx lane memVT size.
hstk30 edited the summary of this revision. (Show Details)

Looks good. Thanks for putting this together. It might be worth checking the tests in the pre-commit, to make sure they are passing.

llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13927–13931

This can probably be something like

auto *StructTy = cast<StructType>(RetTy);
unsigned VecNum = StructTy->getNumElements();
Type *VecTy = StructTy->getElementType(0);

The cast will then assert that the RetTy is a StructType.

llvm/test/CodeGen/AArch64/arm64-neon-st-lane-aa.ll
2

You might not need -O3, or --check-prefix=CHECK. The check lines could also be generated with update_llc_test_checks. It's a good way to generate consistent tests if it does what you need.

Also a quick comment explaining that the order of the st2 should be before the ldrb to the same address might be useful in the long run.

This test case LLVM.CodeGen/AArch64/GlobalISel::arm64-irtranslator.ll fail, cause the :: intrinsic info not equal s384 != <12 * s32> .
Seem <12 * s32> is more direct? I don't know where it'll affect. @dmgreen

I believe that should be fine, so long as the total size is the same.

hstk30 updated this revision to Diff 553105.Aug 24 2023, 6:42 AM

refactor, pass test case

hstk30 updated this revision to Diff 553131.Aug 24 2023, 7:45 AM

pass test case

Passed the fail test case. Have anything to be improved? @dmgreen

hstk30 marked 2 inline comments as done.Aug 28 2023, 12:06 AM

Hi Eli, take a look. Have anything else to be improved? @efriedma

efriedma added inline comments.Aug 28 2023, 2:12 PM
llvm/lib/Target/AArch64/AArch64ISelLowering.cpp
13919

Please also fix the computation for ld2r etc.

This comment was removed by hstk30.

So, I also want to add some test cases for LD like file multi-vector-store-size.ll,
but I can't construct simple cases.

Can you give me an example for ld, LD1Twov4s ...

If you're just having trouble figuring out how to call the intrinsics from IR, there should be a variety of examples in LLVM and clang regression tests. See, for example, llvm/test/CodeGen/AArch64/arm64-ld1.ll .

Otherwise, I'm not sure what you're asking for.

hstk30 updated this revision to Diff 554298.Aug 29 2023, 6:45 AM

fix ld2r etc, and test case

This revision is now accepted and ready to land.Aug 29 2023, 10:21 AM
hstk30 updated this revision to Diff 554530.Aug 29 2023, 5:05 PM

fix typo error

hstk30 updated this revision to Diff 554673.Aug 30 2023, 5:45 AM

pass test case

dmgreen accepted this revision.Aug 30 2023, 10:27 AM

LGTM. Do you have commit access? If not we are happy to commit it for you, we would just need an "name <email@whathaveyou.com>" for the git attribution. Thanks.

"hstk30 <htsk30@gmail.com>", thanks.

hstk30 updated this revision to Diff 555023.Aug 31 2023, 6:38 AM

refactor

Hi, @dmgreen passed the test cases, please commit it "hstk30 <htsk30@gmail.com>", thanks

This revision was automatically updated to reflect the committed changes.