Ignore "no-frame-pointer-elim" and "no-frame-pointer-elim-non-leaf" in favor of "frame-pointer"

[MaskRay]

Summary

D56351 (included in LLVM 8.0.0) introduced "frame-pointer". All tests
which use "no-frame-pointer-elim" or "no-frame-pointer-elim-non-leaf"
have been migrated to use "frame-pointer".

Implement UpgradeFramePointerAttributes to upgrade the two obsoleted
function attributes for bitcode. Their semantics are ignored.

(I have patches to fix two downstream users: ldc and Julia (lightweight use in jit on Windows and FreeBSD)).

[MaskRay]

+ if (!F.hasFnAttribute("frame-pointer"))
+ return false;

(Forgot to include them..

[ychen]

Probably need to translate old attributes to new attribute in AutoUpgrade.cpp? Otherwise, this may cause problems for existing LTO libraries.

[rnk]

In D71863#1795794, @ychen wrote:

Probably need to translate old attributes to new attribute in AutoUpgrade.cpp? Otherwise, this may cause problems for existing LTO libraries.

+1, let's do that.

[MaskRay]

Implement UpgradeFramePointerAttributes to upgrade the two obsoleted function attributes for bitcode.

[ychen]

LGTM

llvm/docs/ReleaseNotes.rst

 * Windows Control Flow Guard: the ``-cfguard`` option now emits CFG checks on
   indirect function calls. The previous behavior is still available with the 
   ``-cfguard-nochecks`` option. Note that this feature should always be used 
   with optimizations enabled.
 
 * ``Callbacks`` have been added to ``CommandLine Options``.  These can
   be used to validate of selectively enable other options.
 
+* The function attributes ``no-frame-pointer-elim`` and
+  ``no-frame-pointer-elim-non-leaf`` have been replaced by ``frame-pointer``,
+  which has 3 values: ``none``, ``non-leaf``, and ``all``. The values mean what
+  functions should retain frame pointers.
+
 Changes to the LLVM IR
 ----------------------
 
 * Unnamed function arguments now get printed with their automatically
   generated name (e.g. "i32 %0") in definitions. This may require front-ends
   to update their tests; if so there is a script utils/add_argument_names.py
   that correctly converted 80-90% of Clang tests. Some manual work will almost
   certainly still be needed.

llvm/include/llvm/IR/AutoUpgrade.h

 //===----------------------------------------------------------------------===//
 
 #ifndef LLVM_IR_AUTOUPGRADE_H
 #define LLVM_IR_AUTOUPGRADE_H
 
 #include "llvm/ADT/StringRef.h"
 
 namespace llvm {
+  class AttrBuilder;
   class CallInst;
   class Constant;
   class Function;
   class Instruction;
   class MDNode;
   class Module;
   class GlobalVariable;
   class Type;
namespace llvm {
 
   /// Upgrade the loop attachment metadata node.
   MDNode *upgradeInstructionLoopAttachment(MDNode &N);
 
   /// Upgrade the datalayout string by adding a section for address space
   /// pointers.
   std::string UpgradeDataLayoutString(StringRef DL, StringRef Triple);
 
+  /// Upgrade function attributes "no-frame-pointer-elim" and
+  /// "no-frame-pointer-elim-non-leaf" to "frame-pointer".
+  void UpgradeFramePointerAttributes(AttrBuilder &B);
+
 } // End llvm namespace
 
 #endif

llvm/lib/Bitcode/Reader/BitcodeReader.cpp

case bitc::PARAMATTR_GRP_CODE_ENTRY: { // ENTRY: [grpid, idx, a0, a1, ...]
           Attribute::AttrKind Kind;
           if (Error Err = parseAttrKind(Record[++i], &Kind))
             return Err;
           if (Kind == Attribute::ByVal)
             B.addByValAttr(HasType ? getTypeByID(Record[++i]) : nullptr);
         }
       }
 
+      UpgradeFramePointerAttributes(B);
       MAttributeGroups[GrpID] = AttributeList::get(Context, Idx, B);
       break;
     }
     }
   }
 }
 
 Error BitcodeReader::parseTypeTable() {

llvm/lib/CodeGen/TargetOptionsImpl.cpp

 /// optimization should be disabled for the given machine function.
 bool TargetOptions::DisableFramePointerElim(const MachineFunction &MF) const {
   // Check to see if the target want to forcably keep frame pointer.
   if (MF.getSubtarget().getFrameLowering()->keepFramePointer(MF))
     return true;
 
   const Function &F = MF.getFunction();
 
-  // TODO: Remove support for old `fp elim` function attributes after fully
-  //       migrate to use "frame-pointer"
-  if (!F.hasFnAttribute("frame-pointer")) {
-    // Check to see if we should eliminate all frame pointers.
-    if (F.getFnAttribute("no-frame-pointer-elim").getValueAsString() == "true")
-      return true;
-
-    // Check to see if we should eliminate non-leaf frame pointers.
-    if (F.hasFnAttribute("no-frame-pointer-elim-non-leaf"))
-      return MF.getFrameInfo().hasCalls();
-
+  if (!F.hasFnAttribute("frame-pointer"))
     return false;
-  }
-
   StringRef FP = F.getFnAttribute("frame-pointer").getValueAsString();
   if (FP == "all")
     return true;
   if (FP == "non-leaf")
     return MF.getFrameInfo().hasCalls();
   if (FP == "none")
     return false;
   llvm_unreachable("unknown frame pointer flag");
 }
 
 /// HonorSignDependentRoundingFPMath - Return true if the codegen must assume
 /// that the rounding mode of the FPU can change from its default.
 bool TargetOptions::HonorSignDependentRoundingFPMath() const {
   return !UnsafeFPMath && HonorSignDependentRoundingFPMathOption;
 }

llvm/lib/IR/AutoUpgrade.cpp

std::string llvm::UpgradeDataLayoutString(StringRef DL, StringRef TT) {
   Regex R("(e-m:[a-z](-p:32:32)?)(-[if]64:.*$)");
   if (!R.match(DL, &Groups))
     return DL;
 
   SmallString<1024> Buf;
   std::string Res = (Groups[1] + AddrSpaces + Groups[3]).toStringRef(Buf).str();
   return Res;
 }
+
+void llvm::UpgradeFramePointerAttributes(AttrBuilder &B) {
+  StringRef FramePointer;
+  if (B.contains("no-frame-pointer-elim")) {
+    // The value can be "true" or "false".
+    for (const auto &I : B.td_attrs())
+      if (I.first == "no-frame-pointer-elim")
+        FramePointer = I.second == "true" ? "all" : "none";
+    B.removeAttribute("no-frame-pointer-elim");
+  }
+  if (B.contains("no-frame-pointer-elim-non-leaf")) {
+    // The value is ignored. "no-frame-pointer-elim"="true" takes priority.
+    if (FramePointer != "all")
+      FramePointer = "non-leaf";
+    B.removeAttribute("no-frame-pointer-elim-non-leaf");
+  }
+
+  if (!FramePointer.empty())
+    B.addAttribute("frame-pointer", FramePointer);
+}

llvm/test/Bitcode/upgrade-frame-pointer.ll

+;; Test that function attributes "no-frame-pointer-elim" ("true" or "false") and
+;; "no-frame-pointer-elim-non-leaf" (value is ignored) can be upgraded to
+;; "frame-pointer".
+
+; RUN: llvm-as %s -o - | llvm-dis - | FileCheck %s
+
+; CHECK: define void @all0() #0
+define void @all0() "no-frame-pointer-elim"="true" { ret void }
+; CHECK: define void @all1() #1
+define void @all1() #0 { ret void }
+
+; CHECK: define void @non_leaf0() #2
+define void @non_leaf0() "no-frame-pointer-elim-non-leaf" { ret void }
+; CHECK: define void @non_leaf1() #3
+define void @non_leaf1() #1 { ret void }
+
+; CHECK: define void @none() #4
+define void @none() "no-frame-pointer-elim"="false" { ret void }
+
+;; Don't add "frame-pointer" if neither "no-frame-pointer-elim" nor
+;; "no-frame-pointer-elim-non-leaf" is present.
+; CHECK: define void @no_attr() {
+define void @no_attr() { ret void }
+
+attributes #0 = { readnone "no-frame-pointer-elim"="true" "no-frame-pointer-elim-non-leaf" }
+attributes #1 = { readnone "no-frame-pointer-elim"="false" "no-frame-pointer-elim-non-leaf" }
+
+;; Other attributes (e.g. readnone) are unaffected.
+; CHECK: attributes #0 = { "frame-pointer"="all" }
+; CHECK: attributes #1 = { readnone "frame-pointer"="all" }
+; CHECK: attributes #2 = { "frame-pointer"="non-leaf" }
+; CHECK: attributes #3 = { readnone "frame-pointer"="non-leaf" }
+; CHECK: attributes #4 = { "frame-pointer"="none" }