[PATCH] D77248: [llvm][IR] Add dso_local_equivalent Constant

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Wed Nov 11 15:42:02 PST 2020


MaskRay added a comment.

Apologies for my late reply. A couple of comments inlined

> See RFC for more details.

Neither the RFC nor the patch mentions the immediate use case. It is possible that the merit is buries in a deep message but for a few feature the description itself should justify its value (dblaikie mentioned that:

> We should do this because I need it" (we shouldn't be implementing features for especially niche use cases/if they don't generalize) isn't always a compelling motivation but "we should do this because someone might need it" isn't either (we shouldn't be implementing features that have no users).

Please add the use cases.



================
Comment at: llvm/include/llvm-c/Core.h:2171
 LLVMValueRef LLVMBlockAddress(LLVMValueRef F, LLVMBasicBlockRef BB);
+LLVMValueRef LLVMDSOLocalEquivalent(LLVMValueRef F);
 
----------------
This is not defined. If you don't add a C implementation, please drop the declaration here and above


================
Comment at: llvm/include/llvm/IR/Constants.h:892
+/// Wrapper for a function that represents a value that
+/// functionally represents the original variable. This can be a function,
+/// global alias to a function, or an ifunc.
----------------
The comment says "function" then "variable". Is "variable" a typo?


================
Comment at: llvm/lib/CodeGen/DSOLocalEquivalentLowering.cpp:20
+
+namespace llvm {
+
----------------
https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions


================
Comment at: llvm/lib/CodeGen/DSOLocalEquivalentLowering.cpp:51
+    if (Triple(M.getTargetTriple()).supportsCOMDAT())
+      Stub->setComdat(M.getOrInsertComdat(StubName));
+  }
----------------
ELF has comdat but does not need this pass.
Mach-O needs this pass but does not have comdat.

I assume that this code is untested.


================
Comment at: llvm/test/CodeGen/X86/dso_local_equivalent.ll:4
+
+; Ensure that writing to an object file works since the object file writer has
+; more restrictions than the assembly printer
----------------
This comment appears to be unrelated to the main purpose of the test. A file-level comment is needed to explain why darwin is picked (it does not support the lowering)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77248



More information about the llvm-commits mailing list