[PATCH] D108478: [llvm][IR] Add no_cfi constant

[Nick Desaulniers via Phabricator via llvm-commits]

nickdesaulniers added a comment.

I think you might want to update the syntax files for the various text editors. See:

- llvm/utils/vim/syntax/llvm.vim
- llvm/utils/emacs/llvm-mode.el
- llvm/utils/kate/llvm.xml
- llvm/utils/vscode/llvm/syntaxes/ll.tmLanguage.yaml



================
Comment at: llvm/include/llvm/IR/Constants.h:936
+
+  void *operator new(size_t s) { return User::operator new(s, 1); }
+
----------------
plz fix linter warning


================
Comment at: llvm/lib/Transforms/IPO/LowerTypeTests.cpp:1760
+    // Skip block addresses and no_cfi values
+    if (isa<BlockAddress, NoCFIValue>(U.getUser()))
       continue;
----------------
Neat! I didn't know `isa` supported multiple types like this!


================
Comment at: llvm/test/Bitcode/nocfivalue.ll:1-2
+; RUN: llvm-as < %s | llvm-dis | FileCheck %s
+; RUN: verify-uselistorder < %s
+
----------------
If the `<` is not necessary, let's drop it!


================
Comment at: llvm/test/Bitcode/nocfivalue.ll:42
+  ret void
+}
+
----------------
If `@f1`, `@f2`, `@f4`, and `@f5` are "empty", want to simply use `declare` rather than `define`?


================
Comment at: llvm/test/CodeGen/X86/nocfivalue.ll:1
+; RUN: opt -S -lowertypetests < %s | llc -asm-verbose=false | FileCheck %s
+
----------------
Do we need `<`? If not, let's remove.

How does `-asm-verbose=false` change this test?


================
Comment at: llvm/test/CodeGen/X86/nocfivalue.ll:15-25
+define void @f1() !type !0 {
+  ret void
+}
+
+define internal void @f2() !type !0 {
+  ret void
+}
----------------
Can we change these to be simply declarations rather then empty function definitions?


================
Comment at: llvm/test/Transforms/LowerTypeTests/nocfivalue.ll:9-22
+; CHECK: define void @f1()
+define void @f1() !type !0 {
+  ret void
+}
+
+; CHECK: define internal void @f2()
+define internal void @f2() !type !0 {
----------------
declare?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D108478