Actually enable the AIX Extended Altivec ABI in LLVM#153784
Actually enable the AIX Extended Altivec ABI in LLVM#153784Gelbpunkt wants to merge 1 commit intorust-lang:mainfrom
Conversation
As far as I can tell, the intent for the powerpc64-ibm-aix target was to enable the AIX Extended Altivec ABI, since `abi` in the target spec was set to `vec-extabi`. This field however has no effect on LLVM codegen and the AIX vector ABI is not set via `llvm_abiname`, but rather enabled in the global TargetOptions struct. Since we only have one AIX target and I believe it should be using the newer ABI, we can unconditionally enable the target option if the target triple is an AIX triple.
rustbot
added
A-LLVM
|
|
|
also cc @daltenty, @gilamn5tr and @amy-kwan as the target maintainers for AIX |
|
Wait, wat? Why is this a TargetMachine option and not controlled via |
I think this will break with LTO, yes. I'm not sure why it is implemented this way in LLVM, it was introduced in llvm/llvm-project@c92f29b |
| // We only support the AIX Extended Altivec ABI and not the legacy AIX ABI. | ||
| // This is required for using the non-volatile vector registers. | ||
| if (Trip.isOSAIX()) | ||
| Options.EnableAIXExtendedAltivecABI = true; |
There was a problem hiding this comment.
Can you add a comment here explaining that abi: Abi::VecExtAbi is correct because we hard-code that for all AIX targets in LLVMRustCreateTargetMachine?
I was wondering the same thing and I am happy that I am not the only one who finds this to be a poor interface choice.^^ This seems quite hacky but then it seems we already have some That said |
|
☔ The latest upstream changes (presumably #153684) made this pull request unmergeable. Please resolve the merge conflicts. |
|
Thanks for looking into this. I'm not sure though that the consequences of enabling the extended vector ABI we're completely consider though when Mainly there are some know safety concerns linking against legacy modules which may fail to respect the ABI (as documented here): I think we'll want to take a closer look at what we are depending on to make sure there's no exposure to such issue before enabling this. |
5 participants
As far as I can tell, the intent for the powerpc64-ibm-aix target was to enable the AIX Extended Altivec ABI, since
abiin the target spec was set tovec-extabi. This field however has no effect on LLVM codegen and the AIX vector ABI is not set viallvm_abiname, but rather enabled in the global TargetOptions struct.Since we only have one AIX target and I believe it should be using the newer ABI, we can unconditionally enable the target option if the target triple is an AIX triple.
r? @RalfJung perhaps? I don't think anyone in the team has AIX expertise
Also cc @nikic, I think this is a bit ugly on the LLVM side, is there perhaps a better way?