[PATCH] D79708: [clang][BFloat] add NEON emitter for bfloat

[Ties Stuij via Phabricator via cfe-commits]

stuij marked 6 inline comments as done.
stuij added inline comments.


================
Comment at: clang/include/clang/Basic/arm_bf16.td:1
+//===--- arm_fp16.td - ARM FP16 compiler interface ------------------------===//
+//
----------------
SjoerdMeijer wrote:
> typo: fp16 - > bf16?
> Here, and a few more places, or is it intentional? If so, I guess that can be a bit confusing?
> 
Thanks for that. I went through the patch and I only found the mistake in this file.


================
Comment at: clang/include/clang/Basic/arm_neon_incl.td:293
+
+  string CartesianProductWith = "";
 }
----------------
fpetrogalli wrote:
> What is this for?
Thanks, that is actually part of another patch. I will remove.


================
Comment at: clang/utils/TableGen/NeonEmitter.cpp:2198
 
+static void emitNeonTypeDefs(const std::string& types, raw_ostream &OS) {
+  std::string TypedefTypes(types);
----------------
fpetrogalli wrote:
> Is this related to the changes for bfloat? Or is it a just a refactoring that it is nice to have? If the latter, please consider submitting it as a separate patch. If both refactoring and BF16 related, at the moment it is not possible to see clearly which changes are BF16 specific, so please do submit the refactoring first.
Yes, related to bfloat. We're emitting that code twice now.

I can make a new patch, but I'm not sure if the effort justifies the in my mind small amount of gain in clarity. It's basically just pasting the removed part on the left into this function. If you disagree, tell me, and I will create the extra patch.


================
Comment at: clang/utils/TableGen/NeonEmitter.cpp:2411
+/// is comprised of type definitions and function declarations.
+void NeonEmitter::runFP16(raw_ostream &OS) {
+  OS << "/*===---- arm_fp16.h - ARM FP16 intrinsics "
----------------
SjoerdMeijer wrote:
> I am a bit confused here, we already have a runFP16, I am missing something?
The fairly similar runBF16 fn was added. The way git interprets this is probably a bit confusing. There's still just one runFP16 function.


================
Comment at: clang/utils/TableGen/NeonEmitter.cpp:2416
+        " *\n"
+        " * Permission is hereby granted, free of charge, to any person "
+        "obtaining a copy\n"
----------------
SjoerdMeijer wrote:
> I can't remember the outcome, but I had a discussion with @sdesmalen about this license, if this should be the new or old copyright notice. I believe, but am not certain, that this should be the new one.
I'll change it, and will ping @sdesmalen to be sure.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D79708