[PATCH] D157485: [X86][RFC] Support new feature AVX10

[Phoebe Wang via Phabricator via cfe-commits]

pengfei added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenFunction.cpp:2581
+                                             unsigned VectorWidth) {
+  if (!getTarget().getTriple().isX86() || VectorWidth < 512)
+    return;
----------------
skan wrote:
> Minor suggestion. The code here may be refined to be better extended by other targets, like
> ```
>   llvm::Triple::ArchType ArchType =
>       getContext().getTargetInfo().getTriple().getArch();
> 
>   switch (ArchType) {
>   case llvm::Triple::x86:
>   case llvm::Triple::x86_64: {
>      ....
>    }
>    default:
>     return;
> 
> ```
We have a few place code using `isX86`. I think it's more convenient to use a single condition. We can refactor when necessary.


================
Comment at: llvm/lib/Target/X86/MCTargetDesc/X86MCCodeEmitter.cpp:927
+      !STI.hasFeature(X86::FeatureAVX10_512bit))
+    report_fatal_error("ZMM registers are not supported without AVX10-512BIT");
   switch (TSFlags & X86II::OpPrefixMask) {
----------------
skan wrote:
> skan wrote:
> > -mavx10.1 does not work for assembler. So if such instruction is generated w/o AVX10-512BIT support, it must be compiler's issue instead of user's. An `assert` should be more appropriate here.
> > -mavx10.1 does not work for assembler. So if such instruction is generated w/o AVX10-512BIT support, it must be compiler's issue instead of user's. An `assert` should be more appropriate here.
> 
> Reference: https://llvm.org/docs/CodingStandards.html#assert-liberally
We need to report fatal error for this case even if it's a compiler bug. Otherwise, user may observe the crash issue in runtime and hard to find the reason.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D157485/new/

https://reviews.llvm.org/D157485