[PATCH] D41599: [X86] Lowering X86 avx512 sqrt intrinsics to IR - LLVM

[Mikhail Dvoretckii via Phabricator via llvm-commits]

mike.dvoretsky added inline comments.


================
Comment at: include/llvm/IR/IntrinsicsX86.td:4497
 
-  def int_x86_avx512_mask_sqrt_ss : GCCBuiltin<"__builtin_ia32_sqrtss_round_mask">,
+  def int_x86_avx512_sqrt_ss_mask : GCCBuiltin<"__builtin_ia32_sqrtss_mask">,
         Intrinsic<[llvm_v4f32_ty], [llvm_v4f32_ty, llvm_v4f32_ty, llvm_v4f32_ty,
----------------
craig.topper wrote:
> Why are we renaming intrinsics here? Is this done to purposely exclude the AVX512 intrinsics? Why are we doing that?
It seems to me that this is done to avoid unconditionally generating the intrinsic in CodeGenFunction::EmitBuiltinExpr in CGBuiltin.cpp on the clang side while keeping the intrinsic available in IR for cases where the rounding mode isn't 4 and it's not being lowered. I haven't been able to find other intrinsic-lowering patches that take measures to keep the intrinsic available rather than just deleting it from IR, so I can't say if this change is conventional. If it isn't, then we need to either look into changing the algorithm in EmitBuiltinExpr to check for lowering before checking if llvm supports the intrinsic, or propose a renaming convention for cases like this one. In the latter case I would propose to put "nonlowered" in the names after the target prefix to keep these distinguishable as renamed, rather than aiming for a similar name and confusing people.


https://reviews.llvm.org/D41599