[PATCH] D71167: [Driver] Default to -momit-leaf-frame-pointer for AArch64

[Fangrui Song via Phabricator via cfe-commits]

This revision was automatically updated to reflect the committed changes.
Closed by commit rG074323c84658: [Driver] Default to -momit-leaf-frame-pointer 
for AArch64 (authored by MaskRay).

Changed prior to commit:
  https://reviews.llvm.org/D71167?vs=232714=233906#toc

Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71167

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/cl-options.c
  clang/test/Driver/frame-pointer-elim.c


Index: clang/test/Driver/frame-pointer-elim.c
===
--- clang/test/Driver/frame-pointer-elim.c
+++ clang/test/Driver/frame-pointer-elim.c
@@ -90,7 +90,9 @@
 // WARN-OMIT-LEAF-7S-NOT: warning: optimization flag 
'-momit-leaf-frame-pointer' is not supported for target 'armv7s'
 // WARN-OMIT-LEAF-7S: "-mframe-pointer=non-leaf"
 
-// On the PS4, we default to omitting the frame pointer on leaf functions
+// On AArch64 and PS4, default to omitting the frame pointer on leaf functions
+// RUN: %clang -### -target aarch64 -S %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S -O2 %s 2>&1 | \
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -182,7 +182,7 @@
 // Oy_2: -O2
 
 // RUN: %clang_cl --target=aarch64-pc-windows-msvc -Werror /Oy- /O2 -### -- %s 
2>&1 | FileCheck -check-prefix=Oy_aarch64 %s
-// Oy_aarch64: -mframe-pointer=all
+// Oy_aarch64: -mframe-pointer=non-leaf
 // Oy_aarch64: -O2
 
 // RUN: %clang_cl --target=i686-pc-win32 -Werror /O2 /O2 -### -- %s 2>&1 | 
FileCheck -check-prefix=O2O2 %s
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -603,9 +603,9 @@
   bool OmitFP = A && A->getOption().matches(options::OPT_fomit_frame_pointer);
   bool NoOmitFP =
   A && A->getOption().matches(options::OPT_fno_omit_frame_pointer);
-  bool KeepLeaf =
-  Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
-   options::OPT_mno_omit_leaf_frame_pointer, 
Triple.isPS4CPU());
+  bool KeepLeaf = Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
+   options::OPT_mno_omit_leaf_frame_pointer,
+   Triple.isAArch64() || Triple.isPS4CPU());
   if (NoOmitFP || mustUseNonLeafFramePointerForTarget(Triple) ||
   (!OmitFP && useFramePointerForTargetByDefault(Args, Triple))) {
 if (KeepLeaf)


Index: clang/test/Driver/frame-pointer-elim.c
===
--- clang/test/Driver/frame-pointer-elim.c
+++ clang/test/Driver/frame-pointer-elim.c
@@ -90,7 +90,9 @@
 // WARN-OMIT-LEAF-7S-NOT: warning: optimization flag '-momit-leaf-frame-pointer' is not supported for target 'armv7s'
 // WARN-OMIT-LEAF-7S: "-mframe-pointer=non-leaf"
 
-// On the PS4, we default to omitting the frame pointer on leaf functions
+// On AArch64 and PS4, default to omitting the frame pointer on leaf functions
+// RUN: %clang -### -target aarch64 -S %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S -O2 %s 2>&1 | \
Index: clang/test/Driver/cl-options.c
===
--- clang/test/Driver/cl-options.c
+++ clang/test/Driver/cl-options.c
@@ -182,7 +182,7 @@
 // Oy_2: -O2
 
 // RUN: %clang_cl --target=aarch64-pc-windows-msvc -Werror /Oy- /O2 -### -- %s 2>&1 | FileCheck -check-prefix=Oy_aarch64 %s
-// Oy_aarch64: -mframe-pointer=all
+// Oy_aarch64: -mframe-pointer=non-leaf
 // Oy_aarch64: -O2
 
 // RUN: %clang_cl --target=i686-pc-win32 -Werror /O2 /O2 -### -- %s 2>&1 | FileCheck -check-prefix=O2O2 %s
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -603,9 +603,9 @@
   bool OmitFP = A && A->getOption().matches(options::OPT_fomit_frame_pointer);
   bool NoOmitFP =
   A && A->getOption().matches(options::OPT_fno_omit_frame_pointer);
-  bool KeepLeaf =
-  Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
-   options::OPT_mno_omit_leaf_frame_pointer, Triple.isPS4CPU());
+  bool KeepLeaf = Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
+   options::OPT_mno_omit_leaf_frame_pointer,
+   Triple.isAArch64() || Triple.isPS4CPU());
   if (NoOmitFP || 

[PATCH] D71167: [Driver] Default to -momit-leaf-frame-pointer for AArch64

[Eli Friedman via Phabricator via cfe-commits]

efriedma accepted this revision.
efriedma added a comment.
This revision is now accepted and ready to land.

Okay, LGTM


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71167



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71167: [Driver] Default to -momit-leaf-frame-pointer for AArch64

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

In D71167#1784272 , @efriedma wrote:

> If I'm understanding correctly, this does nothing without D71168 
> ?  And together with D71168 
> , the net change for clang is that 
> "-mno-omit-leaf-frame-pointer" works correctly, but everything else stays the 
> same?


Yes.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71167



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71167: [Driver] Default to -momit-leaf-frame-pointer for AArch64

[Eli Friedman via Phabricator via cfe-commits]

efriedma added a comment.

If I'm understanding correctly, this does nothing without D71168 
?  And together with D71168 
, the net change for clang is that 
"-mno-omit-leaf-frame-pointer" works correctly, but everything else stays the 
same?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71167



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71167: [Driver] Default to -momit-leaf-frame-pointer for AArch64

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

(I got confused over the last weekend.)

@efriedma We still need this driver change, otherwise D71168 
 will cause a functional difference when 
neither -m(no-)omit-leaf-frame-pointer is specified.

https://godbolt.org/z/653p3q


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71167



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71167: [Driver] Default to -momit-leaf-frame-pointer for AArch64

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added subscribers: dyung, ychen.
MaskRay added a comment.

In D71167#1783298 , @sdesmalen wrote:

> The patch looks structurally fine, but I'm missing the argumentation for 
> changing the default and related, the history of why the default is to 
> //not// omit the frame pointer on leaf functions.
>  Can you provide some insight here?


Honestly we probably don't need this change if the consistency with GCC 
regarding -momit-leaf-frame-pointer is not useful. I touched this function in 
D64294  and I hoped isPS4CPU did not have the 
special rule (@dyung @ychen).

I can abondon this patch if you think it is unnecessary. We may only need 
D71168 .


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71167



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71167: [Driver] Default to -momit-leaf-frame-pointer for AArch64

[Sander de Smalen via Phabricator via cfe-commits]

sdesmalen added a comment.

The patch looks structurally fine, but I'm missing the argumentation for 
changing the default and related, the history of why the default is to //not// 
omit the frame pointer on leaf functions.
Can you provide some insight here?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71167



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71167: [Driver] Default to -momit-leaf-frame-pointer for AArch64

[Fangrui Song via Phabricator via cfe-commits]

MaskRay added a comment.

Ping:)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D71167



___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[PATCH] D71167: [Driver] Default to -momit-leaf-frame-pointer for AArch64

[Fangrui Song via Phabricator via cfe-commits]

MaskRay created this revision.
MaskRay added reviewers: c-rhodes, efriedma, rengolin, sdesmalen.
Herald added subscribers: cfe-commits, kristof.beyls.
Herald added a project: clang.

This matches https://gcc.gnu.org/onlinedocs/gcc/AArch64-Options.html

> -momit-leaf-frame-pointer
> -mno-omit-leaf-frame-pointer
> 
>   Omit or keep the frame pointer in leaf functions. The former behavior is 
> the default.

-mno-omit-leaf-frame-pointer is currently a no-op because
TargetOptions::DisableFramePointerElim is only considered for non-leaf
functions.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D71167

Files:
  clang/lib/Driver/ToolChains/Clang.cpp
  clang/test/Driver/frame-pointer-elim.c


Index: clang/test/Driver/frame-pointer-elim.c
===
--- clang/test/Driver/frame-pointer-elim.c
+++ clang/test/Driver/frame-pointer-elim.c
@@ -90,7 +90,9 @@
 // WARN-OMIT-LEAF-7S-NOT: warning: optimization flag 
'-momit-leaf-frame-pointer' is not supported for target 'armv7s'
 // WARN-OMIT-LEAF-7S: "-mframe-pointer=non-leaf"
 
-// On the PS4, we default to omitting the frame pointer on leaf functions
+// On AArch64 and PS4, default to omitting the frame pointer on leaf functions
+// RUN: %clang -### -target aarch64 -S %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S -O2 %s 2>&1 | \
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -603,9 +603,9 @@
   bool OmitFP = A && A->getOption().matches(options::OPT_fomit_frame_pointer);
   bool NoOmitFP =
   A && A->getOption().matches(options::OPT_fno_omit_frame_pointer);
-  bool KeepLeaf =
-  Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
-   options::OPT_mno_omit_leaf_frame_pointer, 
Triple.isPS4CPU());
+  bool KeepLeaf = Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
+   options::OPT_mno_omit_leaf_frame_pointer,
+   Triple.isAArch64() || Triple.isPS4CPU());
   if (NoOmitFP || mustUseNonLeafFramePointerForTarget(Triple) ||
   (!OmitFP && useFramePointerForTargetByDefault(Args, Triple))) {
 if (KeepLeaf)


Index: clang/test/Driver/frame-pointer-elim.c
===
--- clang/test/Driver/frame-pointer-elim.c
+++ clang/test/Driver/frame-pointer-elim.c
@@ -90,7 +90,9 @@
 // WARN-OMIT-LEAF-7S-NOT: warning: optimization flag '-momit-leaf-frame-pointer' is not supported for target 'armv7s'
 // WARN-OMIT-LEAF-7S: "-mframe-pointer=non-leaf"
 
-// On the PS4, we default to omitting the frame pointer on leaf functions
+// On AArch64 and PS4, default to omitting the frame pointer on leaf functions
+// RUN: %clang -### -target aarch64 -S %s 2>&1 | \
+// RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S %s 2>&1 | \
 // RUN:   FileCheck --check-prefix=KEEP-NON-LEAF %s
 // RUN: %clang -### -target x86_64-scei-ps4 -S -O2 %s 2>&1 | \
Index: clang/lib/Driver/ToolChains/Clang.cpp
===
--- clang/lib/Driver/ToolChains/Clang.cpp
+++ clang/lib/Driver/ToolChains/Clang.cpp
@@ -603,9 +603,9 @@
   bool OmitFP = A && A->getOption().matches(options::OPT_fomit_frame_pointer);
   bool NoOmitFP =
   A && A->getOption().matches(options::OPT_fno_omit_frame_pointer);
-  bool KeepLeaf =
-  Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
-   options::OPT_mno_omit_leaf_frame_pointer, Triple.isPS4CPU());
+  bool KeepLeaf = Args.hasFlag(options::OPT_momit_leaf_frame_pointer,
+   options::OPT_mno_omit_leaf_frame_pointer,
+   Triple.isAArch64() || Triple.isPS4CPU());
   if (NoOmitFP || mustUseNonLeafFramePointerForTarget(Triple) ||
   (!OmitFP && useFramePointerForTargetByDefault(Args, Triple))) {
 if (KeepLeaf)
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits