[PATCH] D96931: [LTO] Discard non-prevailing defined symbols in module-level assembly

Teresa Johnson via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Fri Feb 19 08:19:00 PST 2021


tejohnson added a comment.

I don't love the custom pattern matching approach added here - it is hard to follow and seems potentially brittle. If this case is rare, presumably it is rare that we would have a non-empty set of non prevailing asm symbols, so using something heavier weight shouldn't be an issue if that would work and be clearer.

There are no comments right now so it isn't clear to me what this is doing. What is the behavior before and after the patch? Is this patch fixing an issue created by LTO, or attempting to fix bad input?

I don't follow this comment from the description:

> I find the error message is useful hence this patch.

What happens for ThinLTO?



================
Comment at: llvm/lib/CodeGen/MachineModuleInfo.cpp:173
+  if (Context.hadError())
+    TheModule->getContext().emitError("MC error");
+
----------------
This seems like a good change but orthogonal to the issue you are trying to solve - please extract into a different patch along with the associated test changes.


================
Comment at: llvm/test/LTO/X86/weak-asm.ll:1
+; RUN: split-file %s %t
+; RUN: opt %t/t1.ll -o %t1
----------------
Test case needs some comments.

Also, what is the behavior without this patch?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D96931



More information about the llvm-commits mailing list