This is an archive of the discontinued LLVM Phabricator instance.

[Clang] Emit "min-legal-vector-width" attribute for X86 only
ClosedPublic

Authored by pengfei on Dec 9 2022, 2:49 AM.

Details

Summary

This is an alternative way of D139627 suggested by Craig. Creently only X86 backend uses this attribute. Let's just emit for X86 only.

Diff Detail

Event Timeline

pengfei created this revision.Dec 9 2022, 2:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2022, 2:49 AM
pengfei requested review of this revision.Dec 9 2022, 2:49 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 9 2022, 2:49 AM

I really think only X86 is using this.

TargetCodeGenInfo::setTargetAttributes already exists as a place for targets to emit their custom attributes, x86 could emit it there

pengfei updated this revision to Diff 483751.Dec 17 2022, 7:59 AM

I really think only X86 is using this.

I'm still not sure of that. Please see the diff in AArch64/ARM tests.

TargetCodeGenInfo::setTargetAttributes already exists as a place for targets to emit their custom attributes, x86 could emit it there

It's an attractive suggestion. But it's not easy to move it into target code because the value is not a static one but calculated during codegen. As I explained in LangRef, the value is max(maxVectorWidthInArgsRet(CurFn), maxVectorWidthInArgsRet(Callee0), maxVectorWidthInArgsRet(Callee1), ...). We may need to copy/paste a lot of codegen code there to achieve the same goal.

I really think only X86 is using this.

I'm still not sure of that. Please see the diff in AArch64/ARM tests.

Most of them were done by me in this commit https://reviews.llvm.org/D52441. I either did it for extra coverage or because the tests already checked for #0 and some test cases ended up with #1 due to the different vector lengths.

pengfei updated this revision to Diff 483797.Dec 17 2022, 7:08 PM

I really think only X86 is using this.

I'm still not sure of that. Please see the diff in AArch64/ARM tests.

Most of them were done by me in this commit https://reviews.llvm.org/D52441. I either did it for extra coverage or because the tests already checked for #0 and some test cases ended up with #1 due to the different vector lengths.

Thanks @craig.topper ! Then I think we can just remove them to recover back.

Herald added a project: Restricted Project. · View Herald TranscriptDec 17 2022, 7:08 PM

Please update the title

pengfei retitled this revision from [Clang] Don't emit "min-legal-vector-width"="0" for AMDGPU to [Clang] Emit "min-legal-vector-width" attribute for X86 only.Dec 18 2022, 11:06 PM
pengfei edited the summary of this revision. (Show Details)

Please update the title

Thanks for reminding!

This revision is now accepted and ready to land.Dec 19 2022, 12:10 PM
This revision was landed with ongoing or failed builds.Dec 20 2022, 7:57 PM
This revision was automatically updated to reflect the committed changes.
arsenm added inline comments.Dec 21 2022, 10:19 AM
llvm/docs/LangRef.rst
2235–2241

This still should be documented somewhere. I don't know if we document any target attributes specifically in the langref

pengfei added inline comments.Dec 22 2022, 5:36 AM
llvm/docs/LangRef.rst
2235–2241

Added in D139784, PTAL.