This is an archive of the discontinued LLVM Phabricator instance.

[TargetLowering][RISCV] Fixed a scalable vector issue when lowering [s|u]mul.overflow intrinsics
ClosedPublic

Authored by tangxingxin1008 on Sep 15 2021, 12:19 AM.

Details

Summary
Fixed the vector type issue that where we used getVectorNumElements()
should be replaced by getVectorElementCount() when lowering these
intrinsics.

This is similar to D94149

Signed-off-by: Eric Tang <tangxingxin1008@gmail.com>

Diff Detail

Event Timeline

tangxingxin1008 requested review of this revision.Sep 15 2021, 12:19 AM
Herald added a project: Restricted Project. · View Herald TranscriptSep 15 2021, 12:19 AM
frasercrmck accepted this revision.Sep 16 2021, 1:12 AM

LGTM, thanks!

This revision is now accepted and ready to land.Sep 16 2021, 1:12 AM

Does this affect AArch64 too?

craig.topper retitled this revision from [RISCV] Fixed a scalable vector issue when lowering [s|u]mul.overflow intrinsics to [TargetLowering][RISCV] Fixed a scalable vector issue when lowering [s|u]mul.overflow intrinsics.Sep 18 2021, 4:11 PM

Does this affect AArch64 too?

Yes, I test it on AArch64 sve.
After add this patch, there is no problem on the legal scalable vector that AArch64 sve support, but failed on the vector that need be Widened, such as <n x 1 x i1>.

craig.topper accepted this revision.Sep 22 2021, 11:48 AM

LGTM. Would be nice if you added AArch64 test cases that would fail without this patch as well.

Add AArch64 sve test case. Address Craig Topper's suggestion.

tangxingxin1008 reopened this revision.Nov 16 2021, 2:47 AM

Sorry, I don't know how to merged this patch, and get a wrong way to close this revision. Please forgive me for my mistake.

This revision is now accepted and ready to land.Nov 16 2021, 2:47 AM

Hi @craig.topper @frasercrmck , I don’t have commit access, can you land this patch for me? Thanks.

This revision was landed with ongoing or failed builds.Nov 18 2021, 2:25 AM
This revision was automatically updated to reflect the committed changes.

Hi @craig.topper @frasercrmck , I don’t have commit access, can you land this patch for me? Thanks.

Sure, no problem. I even got to learn how to check out and land other people's patches using arcanist, so that was helpful. Thanks for the patch @tangxingxin1008!

Hi, the tests added in this commit seem to be failing on some buildbots. E.g. https://lab.llvm.org/buildbot/#/builders/109/builds/26584

Hi, the tests added in this commit seem to be failing on some buildbots. E.g. https://lab.llvm.org/buildbot/#/builders/109/builds/26584

Hey. Sorry! Thanks for reporting this. I did check out, build & test this patch locally before pushing so I'm not sure what I did wrong. I'll see if it's as simple as re-running the update_llc_test_checks.py script.

Hi, the tests added in this commit seem to be failing on some buildbots. E.g. https://lab.llvm.org/buildbot/#/builders/109/builds/26584

Just pushed https://reviews.llvm.org/rGe1acbda158b3 which should sort the issue out. Sorry again.

Hi, the tests added in this commit seem to be failing on some buildbots. E.g. https://lab.llvm.org/buildbot/#/builders/109/builds/26584

Just pushed https://reviews.llvm.org/rGe1acbda158b3 which should sort the issue out. Sorry again.

No problem. The tests pass for me locally now, thanks!

Thank you for all your help! @frasercrmck