[ARM64] Maintain compatibility with armv7 intrinsics.

[llvmbot]

Bugzilla Link	19395
Resolution	INVALID
Resolved on	Apr 23, 2014 13:46
Version	trunk
OS	Windows NT
Blocks	llvm/llvm-project#19766
Reporter	LLVM Bugzilla Contributor
CC	@TNorthover

Extended Description

Patterns need to be added to support the selection of legacy ARMv7 intrinsics in ARM64InstrInfo.td. Similar code was added to AArch64InstrNEON.td.

[llvmbot]

Hi Tim,

At the end it just took us a couple of hours to do the fixes. I think we can leave things as they are now. When the merge is complete we will fix our internal code base once more, no problem.

You can close this bug.

Thanks, Ana.

[TNorthover]

Would you like me to do the renaming, or would it be easier for you to do it as while switching over? I'm quite happy either way.

[llvmbot]

I concur with Ana; closing this bug as invalid. If we could do the renaming for those few intrinsics that would be great, Tim. Thanks!

[TNorthover]

After the merge, I probably will rename all the @​llvm.arm64 intrinsics to @​llvm.aarch64 for consistency.

But there's no particular reason it couldn't be done now (at least for the ones you care about). It's a brief bit of ugliness, but it's going to disappear very quickly and if it saves work in the long term that's fine by me.

[llvmbot]

Hello Tim, Jim, Chad,

Just want to clarify we all understand where this issue comes from.

In AArch64 we reuse arm target intrinsics as much as possible and only create aarch64 target intrinsics when needed to map to new AArch64 hardware instructions.

As a result some custom passes we have transform the IR and can generate code that have arm and aarch64 target intrinsic calls. That is why this bug was opened.

We can fix our custom passes to use arm64 target intrinsics and IR pattern (when the target intrinsic is not defined and such pattern can be written).

At least from our side, we do not need you to add support for arm target intrinsics. Not sure if other customers of AArch64 have the same problem.

The only concern I have is how many times we will have to do this fix.

Right now we can change the code to use arm64 namespace.

When the final merged backend is in place, do you plan to rename arm64 target intrinsics to use aarch64 namespace?

That would cause our custom passes to break once again. Though the fix is easy, it is a bit of nuisance.

So please let me know what to expect in the final merged backend.

Here is a list of intrinsics we are currently using and will change to arm64 namespace. If you plan to rename arm64 target intrinsics to use aarch64 namespace, maybe you could start with these soon and save us some time.

arm_neon_vpadd
arm_neon_vpmaxu arm_neon_vpmaxs arm_neon_vpminu arm_neon_vpmins
arm_neon_vmaxu
arm_neon_vmaxs
arm_neon_vminu
arm_neon_vmins
arm_neon_vld2
arm_neon_vld3
arm_neon_vld4
arm_neon_vst2
arm_neon_vst3
arm_neon_vst4

This bug can be closed in my opinion.

[TNorthover]

Yep, there would probably be no type issues with the v7 intrinsics. (I was thinking of the various "<1 x iN>" types that got added to AArch64).

[llvmbot]

Having a mapping from all of the intrinsics is a much bigger deal. What we're considering here is adding support for some of the v7 intrinsics to help folks with the transition.

[llvmbot]

To be clear, I'm primarily interested in maintaining support for ARMv7 intrinsics in the ARM64 backend, however.

[llvmbot]

The original idea was just the ARMv7 intrinsics, but you could imagine also converting the AArch64 intrinsics into the ARM64 intrinsics during and after the merge.

Perhaps, Tim actually meant to say ARM, however?

[llvmbot]

We're just talking about the armv7 intrinsics here, right?

[llvmbot]

Tim, would you mind providing an example of an AArch64 intrinsics that has illegal types for ARM64? Just so I have an idea of what you're referring too.

Also, if we perform it at the IR level, I imagine it would have to be a separate pass. The instcombine passes aren't guaranteed to run, but the auto-upgrading must always occur.

[TNorthover]

I strongly suspect it would have to be an IR level pass since there are AArch64 intrinsics with types that are illegal for ARM64. Intrinsics with illegal types really don't go down well in CodeGen.

Getting something that's correct by that method should be fairly straight-forward, if tedious. Getting the same performance as using the backend's native intrinsics is a slight risk: I think DAG combine should be able to take care of most of the interface differences, but it might miss one or two.

Longer term, it's probably worse for maintenance than a well thought-out set of shared intrinsics. But I suppose we can deal with that issue when we come to it.

[llvmbot]

Having a combine to handle these could. That will help us make the transition for existing clients of AArch64 smoother while allowing the codegen patterns and such to not have to worry about it all.

I don't have strong feelings about it being a DAGCombine vs. InstCombine. As long as it's before codegen and allows other ARM64 DAGCombines that may want to work on intrinsics to fire, it should be fine.

[llvmbot]

I do like the idea of a stable, shared, AArch32 & AArch64 interface to the LLVM intrinsics, but not at the cost of hindering future development of the backend. Thus, I'm not so keen on creating generic intrinsics between the various ARM backends.

What would you two think of an auto-upgrade pass that would translate the ARMv7 intrinsics into the ARM64 intrinsics? This could be done as an instcombine, DAG combine, or in tablegen. Personally, I think a DAG combine would be preferred because it would expose the upgraded intrinsics to other target-specific DAG combines.

A secondary advantage of this approach is that it could maintain bitcode compatibility as well. If we auto-upgrade the AArch64 intrinsics as well, we'll be able to support bitcode from the 3.4 time frame after the merge is complete.

Thoughts?

[TNorthover]

On the other hand, doing it now would mean a much simpler path for AArch64 users, so certainly has its advantages.

[TNorthover]

I agree. Do you have a set of intrinsics that you'd like to become generic Chad? We can try to sort out a truly sensible representation for them and port all the targets over.

(Though the work would be substantially diminished if we didn't have to contend with two 64-bit ports; so it might well be worth postponing this until after the merge).

[llvmbot]

OK. If we really want to share those, then we should look into turning them into truly target independent intrinsics. Having one backend just use intrinsics definitions that come from another backend is a pretty nasty layering violation.

There are, at least some of 'em, ARM (as a family of targets) specific enough that fully target independent intrinsics doesn't make much sense either, though. We need a middle ground. A way to specify, outside of either target, a family of definitions that are shared by both but restricted such that it's clear that they're not needed for other completely separate targets. In low level implementation practice, this is likely to boil down to just a namespace that is neither ARM nor ARM64. Needs some thought about how to get the code organized, both in which stuff is in which files and in how tblgen sees things, to make that happen, though.

That is, it's not the high level idea of sharing intrinsics that bothers me, but that one target is referring to objects that are defined for and in another. If we're going to share them, we should reorganize them such that it's explicit and tidy rather than just convenient.

If you want to push for this, can you have a think on it and write up a draft proposal? Tim and I are often on IRC still, too, if you want to chat.

[llvmbot]

I'm not sure I follow what you're asking for, Chad. The user-level code in arm_neon.h should have full support for the armv7 intrinsics in the arm64 backend. If they don't (with a few exceptions for the TBL ones that are implementation defined, IIRC), it's a bug.

Or are you talking about the intrinsics in the IR? Those aren't shared, and shouldn't be unless we define a set of truly target independent intrinsics for some of the shared operations.

Hi Jim, I'm referring to the intrinsics in the IR. We do in fact have other libraries emitting these IR intrinsics. Because the ARM and AArch64 backends shared the IR intrinsics this hasn't been a problem until now.

[llvmbot]

I'm not sure I follow what you're asking for, Chad. The user-level code in arm_neon.h should have full support for the armv7 intrinsics in the arm64 backend. If they don't (with a few exceptions for the TBL ones that are implementation defined, IIRC), it's a bug.

Or are you talking about the intrinsics in the IR? Those aren't shared, and shouldn't be unless we define a set of truly target independent intrinsics for some of the shared operations.

[TNorthover]

It sounds like you're proposing a stable, shared, AArch32 & AArch64 interface to these LLVM intrinsics. One that external projects can rely on not to change. (I'm assuming you have more than just Clang emitting them, otherwise this is a non-issue since they develop in lock-step).

I think that would be a lot of work (not just in the beginning, but with ongoing maintenance and support). The risk is that we hobble future backend development with the need to support this particular intrinsic API.

Personally, I'm opposed at the moment. It's certainly outside the scope of any merge.

[TNorthover]

I'm adding Jim to the CC list since I know he has strong opinions about this one (probably stronger than mine even).

[llvmbot]

I thought I would reopen this to start the discussion.

AFAIK, LLVM intrinsics can use any namespace. However, I would argue users of ARM64 will have to change their code to use arm64 prefix. When the ARMv8 backends merge is complete, the users may have to change their code back to use aarch64 prefix depending on how things pan out. Thus, it might be worthwhile to support the ARMv7 intrinsics in the ARM64 backend despite not being a strict requirement. This happens to work in the AArch64 backend because of the reuse of the ARMv7 intrinsics.