改进建议

[wiryls]

最近在阅读介绍及代码，有时会注意到一些需要修复的问题。
考虑到为每个问题开 issue 可能导致刷屏，这里单独使用一个 issue 记录阅读该仓库时发现的问题。

备注：下列建议将使用不同修饰词，其中“考虑”指低优先级的建议，“应该”则是期望慎重考虑的建议。

文档

相关 PR：

• [dipu] add markdownlint and update most markdown files #493
• [both] fix, format and remove spaces in README.md #497

文档风格及 Lint 相关

• 考虑使用 markdown lint 等工具检查格式，目前的 markdown 文档风格较为混乱
• 考虑将 markdown lint 加入到 CI\CD 流程
• 考虑在中文和英文单词之间加入空格
• 考虑在下列情况使用 markdown 的多级有序列表：

  deeplink.framework/dipu/README.md

  Lines 20 to 33 in 8dac466

  	#### 1. Runtime (``csrc/dipu/runtime``):
  	*Runtime* 主要有以下几个部分：
  	##### 1）*Core & Distributed*：
  	PyTorch 把一些基本的设备层接口放到了一个叫 ``c10`` 的目录下，不同的设备接入者需要实现该接口来接入 PyTorch。
  	详见[参考文档](http://blog.ezyang.com/2019/05/pytorch-internals/) 里对于``c10`` 的介绍。
  	DIPU 的这一部分主要就是对 PyTorch 的``c10`` 和``c10d``相关接口的实现，把设备无关的部分抽象出一组运行时基类。目前包含 ``DIPUAllocator``，``DIPUGenerator``，``DIPUStream/Event/Guard``，``ProcessGroupDICL`` 等。这些类会把设备相关的请求代理到 *device* 部分定义的一组设备接口。另外用户也可以继承上述基类，实现并注册自己的子类，实现设备特化的某些行为( 这个能力的支持目前尚待完善)。
  	##### 2）*Device*:
  	包含 ``deviceapis.h`` 和 ``diclapis.h`` 两个接口文件。主要是设备 ``memory/stream/event/communcation`` 相关的接口函数（这部分接口后续有考虑挪到 DIOPI 中，成为 DIOPI 的 *Device* 接口，见上图）。
  	#### 2. Aten (``csrc/dipu/aten``):

  deeplink.framework/dipu/Contributors.md

  Lines 93 to 96 in 8dac466

  	(1) 在 GitHub 的 Pull request 界面创建拉取请求
  	(2) 根据指引修改 PR 描述，以便于其他开发者更好地理解你的修改

• 考虑在 CI/CD 流程中加入 commit message 的 lint 工具 (例如 commitlint)

  deeplink.framework/dipu/Contributors.md

  Line 143 in 8dac466

  3. 每次 commit 时需要提供清晰且有意义 commit 信息

文档错误相关

• 应该使用相对路径 (例如 ./QuickStart.md) 而非绝对路径，目前全部分支的下列链接都指向 404 页面：

  deeplink.framework/dipu/README.md

  Lines 96 to 100 in 3ac9fdd

  	## Learn More
  	* [使用/设备接入教学](https://github.com/DeepLink-org/dipu/blob/main/QuickStart.md)
  	* [常见问题](https://github.com/DeepLink-org/dipu/blob/main/FAQ.md)
  	* [开发者指南](https://github.com/DeepLink-org/dipu/blob/main/Contributors.md)

• 应该是 communicatorimpl.cpp 而非 communiatorimpl.cpp (漏掉了 c)

  deeplink.framework/dipu/QuickStart.md

  Line 281 in 8dac466

  - 如果有多机多卡训练的需求，需要根据`dipu/torch_dipu/csrc_dipu/runtime/device/diclapis.h`中的声明，创建`communiatorimpl.cpp`去根据硬件自己底层的*runtime*接口实现对应的函数

• 此处应该为 main 分支而非 master：

  deeplink.framework/dipu/Contributors.md

  Lines 63 to 75 in 8dac466

  	#### 2. 创建开发分支
  	我们需要基于 master 创建开发分支，建议的分支命名规则为 `username/pr_name`。
  	```shell
  	git checkout -b xxx/refactor_contributing_doc
  	```
  	在后续的开发中，如果本地仓库的 master 分支落后于 upstream 的 master 分支，我们需要先拉取 upstream 的代码进行同步，再执行上面的命令
  	```shell
  	git pull upstream master
  	```

  deeplink.framework/dipu/Contributors.md

  Lines 119 to 129 in 8dac466

  	```shell
  	git fetch --all --prune
  	git rebase upstream/master
  	```
  	或者
  	```shell
  	git fetch --all --prune
  	git merge upstream/master
  	```

文档其它事项

• 考虑在 Contributors.md 加入关于 GitHub flow 相关介绍

代码

CMake 脚本相关

参考 #479

代码风格相关

• 考虑提高编译时的警告级别
• 考虑配置 .clang-format 等方式自动格式化 cpp 代码，确保代码风格一致
• 考虑重构并使用统一的日志打印方式，以提供日志级别、输出位置等功能

  deeplink.framework/dipu/torch_dipu/csrc_dipu/utils/Log.h

  Line 11 in 8dac466

  #define DIPU_LOG std::cout << __FILE__ << ":" << __LINE__ << " "

  deeplink.framework/dipu/torch_dipu/csrc_dipu/aten/RegisterDIPU.cpp

  Line 69 in 8dac466

  printf("--%-50s %-30s \n", ("[" + name + "]:").data(), "dipu_fallback");

  deeplink.framework/dipu/torch_dipu/csrc_dipu/aten/RegisterDIPU.cpp

  Line 77 in 8dac466

  std::cout << "numel: " << tensor.numel() << ", sizes: " << tensor.sizes() << ", stride: " << tensor.strides() << ", is_view: " << tensor.is_view() << ", dtype: " << tensor.dtype()

  deeplink.framework/dipu/torch_dipu/csrc_dipu/profiler/collection.cpp

  Line 574 in 8dac466

  TORCH_WARN(fmt::format(

• 如果 pattern 是错误的，构造 std::regex 将会抛异常，考虑增加健壮性

  deeplink.framework/dipu/torch_dipu/csrc_dipu/aten/RegisterDIPU.cpp

  Line 46 in 8dac466

  bool force_fallback = std::regex_match(opname, std::regex(force_fallback_pattern));

• 考虑使用 std::string 的 empty() 方法增加可读性

  deeplink.framework/dipu/torch_dipu/csrc_dipu/aten/RegisterDIPU.cpp

  Line 37 in 8dac466

  if (force_fallback_operators_list.size() <= 0 || opname == nullptr) {

  deeplink.framework/dipu/torch_dipu/csrc_dipu/aten/RegisterDIPU.cpp

  Line 43 in 8dac466

  if (force_fallback_pattern.size() <= 0) {

• 考虑检查参数，并且在参数为 nullptr 的时候返回错误

  deeplink.framework/dipu/torch_dipu/csrc_dipu/diopirt/diopirt_impl.cpp

  Lines 24 to 27 in 8dac466

  	DIOPI_RT_API diopiError_t diopiGetTensorData(diopiTensorHandle_t pth, void** pptr) {
  	*pptr = (reinterpret_cast<at::Tensor*>(pth))->data_ptr();
  	return diopiSuccess;
  	}

• 考虑将 move\copy operator\constructor 也设置为 private 或者设置为 delete

  deeplink.framework/dipu/torch_dipu/csrc_dipu/profiler/CorrelationIDManager.h

  Lines 23 to 25 in 8dac466

  	private:
  	CorrelationIDManager() = default;

• 考虑将文件 dipu/torch_dipu/csrc_dipu/runtime/core/allocator /DIPUCachingAllocator.h 名称修改为 DIPUCacheAllocator.h 毕竟 class 叫做 CacheAllocator

  deeplink.framework/dipu/torch_dipu/csrc_dipu/runtime/core/allocator/DIPUCachingAllocator.h

  Line 69 in 22457dd

  class DIPU_API CacheAllocator: public c10::Allocator, public MemStats {

• 考虑使用 move 并且检查 deque 是否为空

  deeplink.framework/dipu/torch_dipu/csrc_dipu/runtime/core/allocator/DIPUAsyncResourcePool.h

  Line 36 in 8dac466

  T t = std::get<0>(list_.front());

• 考虑放弃使用 spinlock，参考 Linus 关于用户态 spinlock 的评论

  deeplink.framework/dipu/torch_dipu/csrc_dipu/runtime/core/allocator/DIPUSpinMutex.h

  Lines 9 to 37 in 8dac466

  	class SpinMutex {
  	private:
  	std::atomic<bool> excl_ { false };
  	public:
  	constexpr SpinMutex() noexcept = default;
  	SpinMutex(const SpinMutex&) = delete;
  	void delay() const noexcept {
  	std::this_thread::yield();
  	}
  	void lock() {
  	for (bool exp = false;
  	!excl_.compare_exchange_weak(exp, true, std::memory_order_acq_rel);
  	exp = false) delay();
  	}
  	bool try_lock() {
  	bool exp = false;
  	return
  	excl_.compare_exchange_weak(exp, true, std::memory_order_acq_rel);
  	}
  	void unlock() {
  	excl_.store(false, std::memory_order_release);
  	}
  	};

  如果一定要用的话，建议实现时使用 std::atomic_flag 而不是 std::atomic<bool>

代码错误相关

• 这些代码无法保证并发安全，如果需要并发调用，则应该修复

  deeplink.framework/dipu/torch_dipu/csrc_dipu/base/DIPUGlobals.cpp

  Lines 22 to 26 in 8dac466

  	static std::atomic_bool called(false);
  	if (called == true) {
  	return;
  	}
  	called = true;

  deeplink.framework/dipu/torch_dipu/csrc_dipu/base/DIPUGlobals.cpp

  Lines 35 to 39 in 8dac466

  	static std::atomic_bool called(false);
  	if (called == true) {
  	return;
  	}
  	called = true;

  deeplink.framework/dipu/torch_dipu/csrc_dipu/diopirt/diopirt_impl.cpp

  Lines 15 to 22 in 8dac466

  	DIOPI_RT_API const char* diopiGetVersion() {
  	static bool inited = false;
  	if (!inited) {
  	inited = true;
  	snprintf(diopiVersion, sizeof(diopiVersion), "DIOPI Version: %d.%d.%d", DIOPI_VER_MAJOR, DIOPI_VER_MINOR, DIOPI_VER_PATCH);
  	}
  	return diopiVersion;
  	}

• 如果 DIPU_RAW_ALLOCATOR_MIN_ALLOCATE_SIZE 为 0 那么 allocateSize 将一直为 0，应该加入一些检查避免这种情况

  deeplink.framework/dipu/torch_dipu/csrc_dipu/runtime/core/allocator/DIPURawCachingAllocator.cpp

  Lines 37 to 46 in 8dac466

  	static const size_t kMinAllocationSize = [](){
  	size_t size = 512;
  	const char* env = std::getenv("DIPU_RAW_ALLOCATOR_MIN_ALLOCATE_SIZE");
  	if (env != nullptr) {
  	size = std::atoi(env);
  	}
  	return size;
  	}();
  	size_t allocateSize = ((nbytes - 1) | (kMinAllocationSize - 1)) + 1;
  	return allocateSize;

  另外，不太确定这段函数分配的空间大小是否符合期望，当 kMinAllocationSize = 10 nbytes = 21 时，allocateSize 为 30 (参考这段测试)？除此之外，这里的分配器是否应该考虑内存对齐？

To be continued.