<div dir="ltr"><br><br><div class="gmail_quote"><div dir="ltr">On Wed, Oct 26, 2016 at 5:12 PM Peter Collingbourne <<a href="mailto:peter@pcc.me.uk">peter@pcc.me.uk</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">pcc added inline comments.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:669<br class="gmail_msg">
+  if (!Dest->hasLocalLinkage())<br class="gmail_msg">
+    OS << ".globl " << Dest->getName() << "\n";<br class="gmail_msg">
+  OS << ".type " << Dest->getName() << ", function\n";<br class="gmail_msg">
----------------<br class="gmail_msg">
eugenis wrote:<br class="gmail_msg">
> eugenis wrote:<br class="gmail_msg">
> > pcc wrote:<br class="gmail_msg">
> > > I wonder whether we want to escape these names? This appears to be the escaping we'd need to use:<br class="gmail_msg">
> > > <a href="http://llvm-cs.pcc.me.uk/lib/MC/MCSymbol.cpp#53" rel="noreferrer" class="gmail_msg" target="_blank">http://llvm-cs.pcc.me.uk/lib/MC/MCSymbol.cpp#53</a><br class="gmail_msg">
> > ><br class="gmail_msg">
> > > Maybe it's not necessary though since we pretty much expect all names we see to satisfy `MCAsmInfo::isValidUnquotedName`.<br class="gmail_msg">
> > Looks like we should escape the names.<br class="gmail_msg">
> > It's easy to call a function "\n" with __asm__ in the declaration, and that breaks the jumptable code.<br class="gmail_msg">
> ><br class="gmail_msg">
> It appears that neither llvm-mc nor gnu as support quoted identifier properly.<br class="gmail_msg">
> Gnu as allows  a quoted string in, for example, .globl directive. Llvm-mc does too, but it forgets to unescape it, so, say, "\n" turns into a 2-character string when saves as textual asm and read back.<br class="gmail_msg">
><br class="gmail_msg">
> Neither support quoted strings in expressions we need to declare aliases, like<br class="gmail_msg">
>   "\n" = .jumptable + 4.<br class="gmail_msg">
><br class="gmail_msg">
> I think I'll just make it a fatal error to apply cfi-icall to any name that requires escaping.<br class="gmail_msg">
><br class="gmail_msg">
Makes sense, I suppose. Though I think you'll also need to apply IR mangling (`Mangler::getNameWithPrefix`).<br class="gmail_msg">
<br class="gmail_msg"></blockquote><div><br></div><div>This set of APIs is pretty bad. See my recent patches to try to unify it. I'd appreciate trying to use TLOF->getNameWithPrefix if you can rather than calling into the mangler specifically.</div><div> </div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:693<br class="gmail_msg">
<br class="gmail_msg">
+static bool isValidUnquotedName(StringRef Name) {<br class="gmail_msg">
+  if (Name.empty())<br class="gmail_msg">
----------------<br class="gmail_msg">
Maybe `isValidAsmUnquotedName` or something?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: lib/Transforms/IPO/LowerTypeTests.cpp:709<br class="gmail_msg">
 /// for the functions, build the bit sets and lower the llvm.type.test calls.<br class="gmail_msg">
-void LowerTypeTestsModule::buildBitSetsFromFunctionsX86(<br class="gmail_msg">
+void LowerTypeTestsModule::buildBitSetsFromFunctionsNative(<br class="gmail_msg">
     ArrayRef<Metadata *> TypeIds, ArrayRef<Function *> Functions) {<br class="gmail_msg">
----------------<br class="gmail_msg">
This function should have a FIXME to replace the inline asm with some better representation.<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
================<br class="gmail_msg">
Comment at: test/Transforms/LowerTypeTests/function.ll:1-4<br class="gmail_msg">
+; RUN: opt -S -lowertypetests -mtriple=i686-unknown-linux-gnu < %s | FileCheck --check-prefix=X86 %s<br class="gmail_msg">
+; RUN: opt -S -lowertypetests -mtriple=x86_64-unknown-linux-gnu < %s | FileCheck --check-prefix=X86 %s<br class="gmail_msg">
+; RUN: opt -S -lowertypetests -mtriple=arm-unknown-linux-gnu < %s | FileCheck --check-prefix=ARM %s<br class="gmail_msg">
+; RUN: opt -S -lowertypetests -mtriple=aarch64-unknown-linux-gnu < %s | FileCheck --check-prefix=ARM %s<br class="gmail_msg">
----------------<br class="gmail_msg">
Maybe use `check-prefixes` here to reduce duplication below?<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
Repository:<br class="gmail_msg">
  rL LLVM<br class="gmail_msg">
<br class="gmail_msg">
<a href="https://reviews.llvm.org/D25927" rel="noreferrer" class="gmail_msg" target="_blank">https://reviews.llvm.org/D25927</a><br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
<br class="gmail_msg">
</blockquote></div></div>