[clang][NFC] Remove redundant SPIR/SPIR-V TargetInfo types - #23055
[clang][NFC] Remove redundant SPIR/SPIR-V TargetInfo types#23055schittir wants to merge 5 commits into
Conversation
The six Microsoft* device targets in SPIR.h differ from their Windows* bases only in getTargetDefines(): the two x86-32 ones add _M_IX86, the two x86-64 ones add _M_X64/_M_AMD64, and the two AArch64 ones override getTargetDefines() solely to forward to the base. Those macros are already predefined from the host target via -aux-triple, so the device-side copies are redundant. Requested by Tom Honermann in intel#22942 (comment)
…Target With the Microsoft-specific SPIR/SPIR-V TargetInfo types gone, the `default:` and `case llvm::Triple::MSVC:` labels select the same target, so the four `switch (HT.getEnvironment())` blocks no longer distinguish anything.
schittir
changed the title
schittir
changed the title
tahonermann
left a comment
tahonermann
left a comment
There was a problem hiding this comment.
Looks good, but I'm wondering if there is more that can be removed.
| class LLVM_LIBRARY_VISIBILITY WindowsX86_64_SPIR64TargetInfo | ||
| : public WindowsTargetInfo<SPIR64TargetInfo> { |
There was a problem hiding this comment.
Do we need to retain the Windows variants here? I was expecting them to be removable as well. Perhaps the places where these are still constructed can be replaced with, e.g., WindowsTargetInfo<SPIR64TargetInfo> matching what is done on Linux?
There was a problem hiding this comment.
Properly doing this involves a pull down of upstream's host-delegating getBuiltinVaListKind, adding a protected adoptHostPointerTypes() called from the four concrete SPIR/SPIR-V constructors (guarded on matching pointer width, deliberately not from logical SPIRVTargetInfo), and a few more changes to CC_X86VectorCall, moving sse/sse2 into the base etc.
I tried to keep this PR NFC but would it make sense to just go ahead and do these?
There was a problem hiding this comment.
I think the cleaner option from a git history perspective would be to wait for the pulldown to complete and then remove all of the obsolete cases. But I'm not opposed to going in stages either if that is your preference.
There was a problem hiding this comment.
I think all the FPGA related target support can be removed too. Would you mind removing those too?
Curiously, the FPGA targets appear to be named differently in xmain vs intel/llvm. e.g., SPIR64INTELFpgaTargetInfo vs SPIR64FPGATargetInfo.
There was a problem hiding this comment.
By all FPGA related target support, do you mean to include the builtins too?
There was a problem hiding this comment.
Are you referring to builtin functions? They presumably should be removed and, as far as I know, might have alreyad been removed. @premanandrao would be one to check with.
| } | ||
| assert(HT.getArch() == llvm::Triple::x86 && | ||
| "Unsupported host architecture"); | ||
| return std::make_unique<WindowsX86_32SPIRTargetInfo>(Triple, Opts); |
There was a problem hiding this comment.
Per one of my other comments, can this be changed to match what is done for Linux?
| return std::make_unique<WindowsX86_32SPIRTargetInfo>(Triple, Opts); | |
| return std::make_unique<WindowsTargetInfo<SPIR32TargetInfo>>(Triple, Opts); |
…e host BaseSPIRTargetInfo already copies LongWidth/LongAlign, DoubleAlign, LongLongAlign, IntMaxType, Int64Type and WCharType from the host target, and every MSVC host supplies exactly the values these constructors re-assigned. Only SizeType/PtrDiffType/IntPtrType are kept, since the base class does not copy those and the SPIR64/SPIRV64 defaults are wrong for a Windows host.
FPGA support has already been removed from the driver: -fintelfpga and -fsycl-targets=spir64_fpga are rejected with err_drv_unsupported_opt_removed, so SPIR64FPGATargetInfo was only reachable through a direct -cc1 -triple spir64_fpga invocation. Follow up for intel#22942
schittir
changed the title
| DoubleAlign = LongLongAlign = 64; | ||
| WCharType = UnsignedShort; |
There was a problem hiding this comment.
These removals are dependent on llvm/llvm-project#208196 having been merged via pulldown, aren't they? Likewise for the other Windows... modification cases.
2 participants
This patch removes the now redundant six Microsoft SPIR/SPIRV device targets.
Follow-up for #22942 (comment)