This is an archive of the discontinued LLVM Phabricator instance.

[SVE] Change some bfloat lane intrinsics to use i32 immediates
ClosedPublic

Authored by david-arm on Nov 28 2022, 3:58 AM.

Details

Summary

Almost all of the other SVE LLVM IR intrinsics take i32 values
for lane indices or other immediates. We should bring the bfloat
intrinsics in line with that. It will also make it easier to
add support for the SVE2.1 float intrinsics in future, since
they reuse the same underlying instruction classes.

I've maintained backwards compatibility with the old i64 variants
and used the autoupgrade mechanism.

Diff Detail

Event Timeline

david-arm created this revision.Nov 28 2022, 3:58 AM
Herald added a project: Restricted Project. · View Herald Transcript
david-arm requested review of this revision.Nov 28 2022, 3:58 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptNov 28 2022, 3:58 AM
david-arm updated this revision to Diff 478494.Nov 29 2022, 2:43 AM
david-arm edited the summary of this revision. (Show Details)
  • Changed patch to use autoupgrade mechanism.
sdesmalen added inline comments.Nov 30 2022, 1:20 AM
llvm/include/llvm/IR/IntrinsicsAArch64.td
2524

do you also want to remove the old intrinsics?

david-arm updated this revision to Diff 478868.Nov 30 2022, 2:48 AM
  • Actually done the upgrade stuff properly this time. :)
david-arm marked an inline comment as done.Nov 30 2022, 2:49 AM
david-arm added inline comments.
llvm/include/llvm/IR/IntrinsicsAArch64.td
2524

Good spot! Turns out that not only had I done this wrong, but I'd also missed out upgrades for bfmlalb/t too. :)

paulwalker-arm added inline comments.Dec 5 2022, 9:01 AM
llvm/include/llvm/IR/IntrinsicsAArch64.td
2524

Having _i32 in the name is confusing because it'll come out as .i32 when printed in IR, which looks like a type suffix but in this case it's actually part of the name.

With that said, do you have to change the name? That seems unfortunate given this is a bug fix.

If it's absolutely necessary then I suggestion using _v2 to signify this is the second version of this intrinsic.

david-arm marked an inline comment as done.Dec 6 2022, 1:30 AM
david-arm added inline comments.
llvm/include/llvm/IR/IntrinsicsAArch64.td
2524

Sadly @paulwalker-arm the auto-upgrade mechanism forbids you from upgrading to an intrinsic of the same name. I don't really know why we have that restriction though, since in theory I could decide to upgrade only when the index is a i64 type. I'll use _v2 then!

david-arm updated this revision to Diff 480451.Dec 6 2022, 6:04 AM

I did try to avoid having to create an intrinsic with a different name, but I was thwarted at every turn! It is with great regret that I report these two problems:

  1. When you write an IR test file containing a declaration of the same intrinsic with a different type and try to use the upgrade mechanism the compiler crashes in llvm::Intrinsic::getDeclaration. This is presumably because we've declared an intrinsic in the module with a different signature to that in IntrinsicsAArch64.td and it seems deeply unhappy about it.
  2. Secondly, in llvm::UpgradeIntrinsicFunction we would also hit the assert assert(F != NewFn && "Intrinsic function upgraded to the same function");

Let us bemoan the evil that befalls us @paulwalker-arm!

paulwalker-arm accepted this revision.Dec 6 2022, 6:41 AM
paulwalker-arm added inline comments.
llvm/include/llvm/IR/IntrinsicsAArch64.td
1517

I suppose you don't actually need to change the name of this class.

llvm/lib/IR/AutoUpgrade.cpp
3977 ↗(On Diff #480451)

This may as well be 4 given we know the exact number of arguments.

This revision is now accepted and ready to land.Dec 6 2022, 6:41 AM
This revision was landed with ongoing or failed builds.Dec 7 2022, 1:20 AM
This revision was automatically updated to reflect the committed changes.