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

[Fangrui Song via Phabricator via llvm-commits]

MaskRay added inline comments.


================
Comment at: llvm/lib/CodeGen/DSOLocalEquivalentLowering.cpp:20
+
+namespace llvm {
+
----------------
MaskRay wrote:
> https://llvm.org/docs/CodingStandards.html#use-namespace-qualifiers-to-implement-previously-declared-functions
Please use `llvm::foo` to define functions/variables in .cpp files


================
Comment at: llvm/lib/CodeGen/DSOLocalEquivalentLowering.cpp:9
+//
+// If this target cannot lower the dso_local_equivalent pass to an MCExpr, the
+// expression is lowered with a generic IR implementation.
----------------
Transform uses of dso_local_equivalent to a function stub for targets which do not support lowering dso_local_equivalent to MCExpr.


================
Comment at: llvm/lib/CodeGen/DSOLocalEquivalentLowering.cpp:30
+    assert(Stub->isDSOLocal() &&
+           "The previous definition of this stub should be dso_local.");
+    return Stub;
----------------
Nit: Strings in assert usually do not need to end with a period


================
Comment at: llvm/lib/CodeGen/DSOLocalEquivalentLowering.cpp:36
+    // Propagate function attributes.
+    Stub = Function::Create(F->getFunctionType(), F->getLinkage(), StubName, M);
+    Stub->setAttributes(F->getAttributes());
----------------
I think the stub should be InternalLinkage (if you want to retain a symbol) or PrivateLinkage (if you don't want a symbol). You would not need hidden visibility below.

If you use a hidden function, and two translation units have dso_local declarations of the same function, the produced stubs will cause a duplicate definition error at link time.


================
Comment at: llvm/test/CodeGen/X86/dso_local_equivalent.ll:10
+; Just ensure that we can write to an object file without error.
+; RUN: llc -filetype=obj -mtriple=x86_64-linux-gnu -relocation-model=pic -data-sections -o - %s
+; RUN: llc -filetype=obj -mtriple=x86_64-apple-darwin -relocation-model=pic -data-sections -o - %s
----------------
`-o -` -> `-o /dev/null`


================
Comment at: llvm/test/CodeGen/X86/dso_local_equivalent.ll:128
+; NO-PLT:         jmp     _extern_func
+; NO-PLT-NOT:  _hidden_func.stub
+; NO-PLT-NOT:  _protected_func.stub
----------------
Use one single `NO-PLT-NOT: .stub`


================
Comment at: llvm/test/CodeGen/X86/dso_local_equivalent.ll:144
+; NO-PLT-COMDAT:      _extern_func.stub:
+; NO-PLT-COMDAT-NOT:  _hidden_func.stub
+; NO-PLT-COMDAT-NOT:  _protected_func.stub
----------------
Use one single `NO-PLT-COMDAT-NOT: .stub`


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D77248