[clang] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (PR #88538)

2024-04-22 Thread Craig Topper via cfe-commits 

https://github.com/topperc closed 
https://github.com/llvm/llvm-project/pull/88538
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (PR #88538)

2024-04-22 Thread Philip Reames via cfe-commits 

https://github.com/preames approved this pull request.

LGTM

https://github.com/llvm/llvm-project/pull/88538
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (PR #88538)

2024-04-19 Thread Craig Topper via cfe-commits 

https://github.com/topperc updated 
https://github.com/llvm/llvm-project/pull/88538

>From a27600bd46a784dbf028dba4153858700d7843af Mon Sep 17 00:00:00 2001
From: Craig Topper 
Date: Fri, 12 Apr 2024 09:57:06 -0700
Subject: [PATCH 1/2] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V

-O0 implies -mrelax-all as an assembler compile time optimization.
-mrelax-all allows the assembler to complete layout in 2 passes
instead of doing iterative branch relaxation.

Jump offsets larger than +/-1MiB require an indirect jump on RISC-V.
This can't be done by the assembler, so we use a branch relaxation
MIR pass and use register scavenging to find a free register.

The conditional branch offsets for RISC-V are also somewhat small
so we support MC layer branch relaxation to make life easier for
assembly programmers. This may also cover up bugs in our function
size estimation in MachineIR.

Enabling -mrelax-all causes the MC layer relaxation to agressively
relax branches. This increases code size and can create cases where
we need an indirect jump, but we can't create one. This leads to
linker failures.

The easiest way to avoid this is to not default to -mrelax-all for
-O0 and sacrifice the compile time optimization. That's what this
patch does.

Fixes #87127
---
 clang/lib/Driver/ToolChains/Clang.cpp | 10 ++
 clang/test/Driver/integrated-as.c |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 766a9b91e3c0ad..856a88bdc3aad4 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -844,6 +844,16 @@ static bool UseRelaxAll(Compilation &C, const ArgList 
&Args) {
   if (Arg *A = Args.getLastArg(options::OPT_O_Group))
 RelaxDefault = A->getOption().matches(options::OPT_O0);
 
+  // RISC-V requires an indirect jump for offsets larger than 1MiB. This cannot
+  // be done by assembler branch relaxation as it needs a free temporary
+  // register. Because of this, branch relaxation is handled by a MachineIR
+  // pass before the assembler. Forcing assembler branch relaxation for -O0
+  // makes the MachineIR branch relaxation inaccurate and it will miss cases
+  // where an indirect branch is necessary. To avoid this issue we are
+  // sacrificing the compile time improvement of using -mrelax-all for -O0.
+  if (C.getDefaultToolChain().getTriple().isRISCV())
+RelaxDefault = false;
+
   if (RelaxDefault) {
 RelaxDefault = false;
 for (const auto &Act : C.getActions()) {
diff --git a/clang/test/Driver/integrated-as.c 
b/clang/test/Driver/integrated-as.c
index d7658fdfd63374..5b79714a2c547e 100644
--- a/clang/test/Driver/integrated-as.c
+++ b/clang/test/Driver/integrated-as.c
@@ -1,4 +1,4 @@
-// XFAIL: target={{.*}}-aix{{.*}}
+// XFAIL: target={{.*}}-aix{{.*}}, target=riscv{{.*}}
 
 // RUN: %clang -### -c -save-temps -integrated-as %s 2>&1 | FileCheck %s
 

>From fa76e26a2d8bda55e17324efabc14913de0a04ed Mon Sep 17 00:00:00 2001
From: Craig Topper 
Date: Fri, 19 Apr 2024 13:34:14 -0700
Subject: [PATCH 2/2] fixup! update test

---
 clang/test/Driver/integrated-as.c | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/clang/test/Driver/integrated-as.c 
b/clang/test/Driver/integrated-as.c
index 5b79714a2c547e..e78fde873cf47f 100644
--- a/clang/test/Driver/integrated-as.c
+++ b/clang/test/Driver/integrated-as.c
@@ -1,10 +1,16 @@
-// XFAIL: target={{.*}}-aix{{.*}}, target=riscv{{.*}}
+// XFAIL: target={{.*}}-aix{{.*}}
 
-// RUN: %clang -### -c -save-temps -integrated-as %s 2>&1 | FileCheck %s
+// RUN: %clang -### -c -save-temps -integrated-as --target=x86_64 %s 2>&1 | 
FileCheck %s
 
 // CHECK: cc1as
 // CHECK: -mrelax-all
 
+// RISC-V does not enable -mrelax-all
+// RUN: %clang -### -c -save-temps -integrated-as --target=riscv64 %s 2>&1 | 
FileCheck %s -check-prefix=RISCV-RELAX
+
+// RISCV-RELAX: cc1as
+// RISCV-RELAX-NOT: -mrelax-all
+
 // RUN: %clang -### -fintegrated-as -c -save-temps %s 2>&1 | FileCheck %s 
-check-prefix FIAS
 
 // FIAS: cc1as

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

[clang] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (PR #88538)

2024-04-19 Thread Fangrui Song via cfe-commits 


@@ -1,4 +1,4 @@
-// XFAIL: target={{.*}}-aix{{.*}}
+// XFAIL: target={{.*}}-aix{{.*}}, target=riscv{{.*}}

MaskRay wrote:

Perhaps the commonplace `--target=x86_64`

https://github.com/llvm/llvm-project/pull/88538
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (PR #88538)

2024-04-19 Thread Craig Topper via cfe-commits 


@@ -1,4 +1,4 @@
-// XFAIL: target={{.*}}-aix{{.*}}
+// XFAIL: target={{.*}}-aix{{.*}}, target=riscv{{.*}}

topperc wrote:

What triple should i use for the -mrelax-all test?

https://github.com/llvm/llvm-project/pull/88538
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (PR #88538)

2024-04-17 Thread Fangrui Song via cfe-commits 


@@ -1,4 +1,4 @@
-// XFAIL: target={{.*}}-aix{{.*}}
+// XFAIL: target={{.*}}-aix{{.*}}, target=riscv{{.*}}

MaskRay wrote:

This causes different behaviors with the default target triple, which is 
probably not perfect.

I think we can change the -mrelax-all check with an explicit target triple and 
then add a `--target=riscv64`.

https://github.com/llvm/llvm-project/pull/88538
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (PR #88538)

2024-04-17 Thread Fangrui Song via cfe-commits 

https://github.com/MaskRay approved this pull request.

I left a comment at 
https://github.com/llvm/llvm-project/issues/87127#issuecomment-2063027106

This is more about a weird -O0 -mrelax-all behavior that RISC-V now opts out, 
which is good on its own, even if the basis looks like a self-inflicted 
problem...

https://github.com/llvm/llvm-project/pull/88538
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (PR #88538)

2024-04-12 Thread via cfe-commits 

llvmbot wrote:



@llvm/pr-subscribers-clang

@llvm/pr-subscribers-clang-driver

Author: Craig Topper (topperc)


Changes

-O0 implies -mrelax-all as an assembler compile time optimization. -mrelax-all 
allows the assembler to complete layout in 2 passes instead of doing iterative 
branch relaxation.

Jump offsets larger than +/-1MiB require an indirect jump on RISC-V. This can't 
be done by the assembler, so we use a branch relaxation MIR pass and use 
register scavenging to find a free register.

The conditional branch offsets for RISC-V are also somewhat small so we support 
MC layer branch relaxation to make life easier for assembly programmers. This 
may also cover up bugs in our function size estimation in MachineIR.

Enabling -mrelax-all causes the MC layer relaxation to agressively relax 
branches. This increases code size and can create cases where we need an 
indirect jump, but we can't create one. This leads to linker failures.

The easiest way to avoid this is to not default to -mrelax-all for -O0 and 
sacrifice the compile time optimization. That's what this patch does.

Fixes #87127

---
Full diff: https://github.com/llvm/llvm-project/pull/88538.diff


2 Files Affected:

- (modified) clang/lib/Driver/ToolChains/Clang.cpp (+10) 
- (modified) clang/test/Driver/integrated-as.c (+1-1) 


``diff
diff --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 766a9b91e3c0ad..856a88bdc3aad4 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -844,6 +844,16 @@ static bool UseRelaxAll(Compilation &C, const ArgList 
&Args) {
   if (Arg *A = Args.getLastArg(options::OPT_O_Group))
 RelaxDefault = A->getOption().matches(options::OPT_O0);
 
+  // RISC-V requires an indirect jump for offsets larger than 1MiB. This cannot
+  // be done by assembler branch relaxation as it needs a free temporary
+  // register. Because of this, branch relaxation is handled by a MachineIR
+  // pass before the assembler. Forcing assembler branch relaxation for -O0
+  // makes the MachineIR branch relaxation inaccurate and it will miss cases
+  // where an indirect branch is necessary. To avoid this issue we are
+  // sacrificing the compile time improvement of using -mrelax-all for -O0.
+  if (C.getDefaultToolChain().getTriple().isRISCV())
+RelaxDefault = false;
+
   if (RelaxDefault) {
 RelaxDefault = false;
 for (const auto &Act : C.getActions()) {
diff --git a/clang/test/Driver/integrated-as.c 
b/clang/test/Driver/integrated-as.c
index d7658fdfd63374..5b79714a2c547e 100644
--- a/clang/test/Driver/integrated-as.c
+++ b/clang/test/Driver/integrated-as.c
@@ -1,4 +1,4 @@
-// XFAIL: target={{.*}}-aix{{.*}}
+// XFAIL: target={{.*}}-aix{{.*}}, target=riscv{{.*}}
 
 // RUN: %clang -### -c -save-temps -integrated-as %s 2>&1 | FileCheck %s
 

``




https://github.com/llvm/llvm-project/pull/88538
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits

[clang] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V (PR #88538)

2024-04-12 Thread Craig Topper via cfe-commits 

https://github.com/topperc created 
https://github.com/llvm/llvm-project/pull/88538

-O0 implies -mrelax-all as an assembler compile time optimization. -mrelax-all 
allows the assembler to complete layout in 2 passes instead of doing iterative 
branch relaxation.

Jump offsets larger than +/-1MiB require an indirect jump on RISC-V. This can't 
be done by the assembler, so we use a branch relaxation MIR pass and use 
register scavenging to find a free register.

The conditional branch offsets for RISC-V are also somewhat small so we support 
MC layer branch relaxation to make life easier for assembly programmers. This 
may also cover up bugs in our function size estimation in MachineIR.

Enabling -mrelax-all causes the MC layer relaxation to agressively relax 
branches. This increases code size and can create cases where we need an 
indirect jump, but we can't create one. This leads to linker failures.

The easiest way to avoid this is to not default to -mrelax-all for -O0 and 
sacrifice the compile time optimization. That's what this patch does.

Fixes #87127

>From a27600bd46a784dbf028dba4153858700d7843af Mon Sep 17 00:00:00 2001
From: Craig Topper 
Date: Fri, 12 Apr 2024 09:57:06 -0700
Subject: [PATCH] [RISCV][clang] Don't enable -mrelax-all for -O0 on RISC-V

-O0 implies -mrelax-all as an assembler compile time optimization.
-mrelax-all allows the assembler to complete layout in 2 passes
instead of doing iterative branch relaxation.

Jump offsets larger than +/-1MiB require an indirect jump on RISC-V.
This can't be done by the assembler, so we use a branch relaxation
MIR pass and use register scavenging to find a free register.

The conditional branch offsets for RISC-V are also somewhat small
so we support MC layer branch relaxation to make life easier for
assembly programmers. This may also cover up bugs in our function
size estimation in MachineIR.

Enabling -mrelax-all causes the MC layer relaxation to agressively
relax branches. This increases code size and can create cases where
we need an indirect jump, but we can't create one. This leads to
linker failures.

The easiest way to avoid this is to not default to -mrelax-all for
-O0 and sacrifice the compile time optimization. That's what this
patch does.

Fixes #87127
---
 clang/lib/Driver/ToolChains/Clang.cpp | 10 ++
 clang/test/Driver/integrated-as.c |  2 +-
 2 files changed, 11 insertions(+), 1 deletion(-)

diff --git a/clang/lib/Driver/ToolChains/Clang.cpp 
b/clang/lib/Driver/ToolChains/Clang.cpp
index 766a9b91e3c0ad..856a88bdc3aad4 100644
--- a/clang/lib/Driver/ToolChains/Clang.cpp
+++ b/clang/lib/Driver/ToolChains/Clang.cpp
@@ -844,6 +844,16 @@ static bool UseRelaxAll(Compilation &C, const ArgList 
&Args) {
   if (Arg *A = Args.getLastArg(options::OPT_O_Group))
 RelaxDefault = A->getOption().matches(options::OPT_O0);
 
+  // RISC-V requires an indirect jump for offsets larger than 1MiB. This cannot
+  // be done by assembler branch relaxation as it needs a free temporary
+  // register. Because of this, branch relaxation is handled by a MachineIR
+  // pass before the assembler. Forcing assembler branch relaxation for -O0
+  // makes the MachineIR branch relaxation inaccurate and it will miss cases
+  // where an indirect branch is necessary. To avoid this issue we are
+  // sacrificing the compile time improvement of using -mrelax-all for -O0.
+  if (C.getDefaultToolChain().getTriple().isRISCV())
+RelaxDefault = false;
+
   if (RelaxDefault) {
 RelaxDefault = false;
 for (const auto &Act : C.getActions()) {
diff --git a/clang/test/Driver/integrated-as.c 
b/clang/test/Driver/integrated-as.c
index d7658fdfd63374..5b79714a2c547e 100644
--- a/clang/test/Driver/integrated-as.c
+++ b/clang/test/Driver/integrated-as.c
@@ -1,4 +1,4 @@
-// XFAIL: target={{.*}}-aix{{.*}}
+// XFAIL: target={{.*}}-aix{{.*}}, target=riscv{{.*}}
 
 // RUN: %clang -### -c -save-temps -integrated-as %s 2>&1 | FileCheck %s
 

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