diff mbox series

[2/2] RISC-V: remove CM_PIC as it doesn't do much

Message ID 20220830174830.224541-3-vineetg@rivosinc.com
State New, archived
Headers
Series PIC cleanup |

Commit Message

Vineet Gupta Aug. 30, 2022, 5:48 p.m. UTC 
  CM_PIC is no longer doing anything directly. Removing it might 
potentially affect USE_LOAD_ADDRESS_MACRO() but seems unlikely. 

Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
---
 gcc/config/riscv/riscv-c.cc   | 4 ----
 gcc/config/riscv/riscv-opts.h | 3 +--
 gcc/config/riscv/riscv.cc     | 2 +-
 3 files changed, 2 insertions(+), 7 deletions(-)

Comments

Palmer Dabbelt Aug. 31, 2022, 2:57 p.m. UTC | #1 
On Tue, 30 Aug 2022 10:48:30 PDT (-0700), Vineet Gupta wrote:
> CM_PIC is no longer doing anything directly. Removing it might
> potentially affect USE_LOAD_ADDRESS_MACRO() but seems unlikely.

At least in the short term, there's kind of a mess here that needs to 
get sorted out but just removing CM_PIC doesn't really get us there.

> Signed-off-by: Vineet Gupta <vineetg@rivosinc.com>
> ---
>  gcc/config/riscv/riscv-c.cc   | 4 ----
>  gcc/config/riscv/riscv-opts.h | 3 +--
>  gcc/config/riscv/riscv.cc     | 2 +-
>  3 files changed, 2 insertions(+), 7 deletions(-)
>
> diff --git a/gcc/config/riscv/riscv-c.cc b/gcc/config/riscv/riscv-c.cc
> index bba72cf77a82..7064fcf142fe 100644
> --- a/gcc/config/riscv/riscv-c.cc
> +++ b/gcc/config/riscv/riscv-c.cc
> @@ -92,13 +92,9 @@ riscv_cpu_cpp_builtins (cpp_reader *pfile)
>        builtin_define ("__riscv_cmodel_medlow");
>        break;
>
> -    case CM_PIC:
> -      /* FALLTHROUGH. */
> -
>      case CM_MEDANY:
>        builtin_define ("__riscv_cmodel_medany");
>        break;
> -
>      }
>
>    if (TARGET_MIN_VLEN != 0)
> diff --git a/gcc/config/riscv/riscv-opts.h b/gcc/config/riscv/riscv-opts.h
> index 85e869e62e3a..ce3237beca7a 100644
> --- a/gcc/config/riscv/riscv-opts.h
> +++ b/gcc/config/riscv/riscv-opts.h
> @@ -34,8 +34,7 @@ extern enum riscv_abi_type riscv_abi;
>
>  enum riscv_code_model {
>    CM_MEDLOW,
> -  CM_MEDANY,
> -  CM_PIC
> +  CM_MEDANY
>  };
>  extern enum riscv_code_model riscv_cmodel;
>
> diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
> index 7c120eaa8e33..a239fe43047c 100644
> --- a/gcc/config/riscv/riscv.cc
> +++ b/gcc/config/riscv/riscv.cc
> @@ -5162,7 +5162,7 @@ riscv_option_override (void)
>    init_machine_status = &riscv_init_machine_status;
>
>    if (flag_pic)
> -    riscv_cmodel = CM_PIC;
> +    riscv_cmodel = CM_MEDANY;
>
>    /* We get better code with explicit relocs for CM_MEDLOW, but
>       worse code for the others (for now).  Pick the best default.  */

I'm fine either way on this one: having CM_PIC gone makes it a bit more 
likely to confuse CM_MEDANY with PIC, but flag_pic is overriding 
riscv_cmodel anyway so this isn't really used and deleting code is 
always a plus.
Vineet Gupta Aug. 31, 2022, 8:39 p.m. UTC | #2 
On 8/31/22 07:57, Palmer Dabbelt wrote:
>>    if (flag_pic)
>> -    riscv_cmodel = CM_PIC;
>> +    riscv_cmodel = CM_MEDANY;
>>
>>    /* We get better code with explicit relocs for CM_MEDLOW, but
>>       worse code for the others (for now).  Pick the best default.  */
>
> I'm fine either way on this one: having CM_PIC gone makes it a bit 
> more likely to confuse CM_MEDANY with PIC, but flag_pic is overriding 
> riscv_cmodel anyway so this isn't really used and deleting code is 
> always a plus.

Indeed this was the most contentious part of removing CM_PIC, but it 
seems this is the way fwd. I'll add Kito's comment from [1] in code to 
make it more explicit.

[1]https://github.com/riscv-non-isa/riscv-c-api-doc/pull/11#issuecomment-686385585 


Thx,
-Vineett
Vineet Gupta Sept. 2, 2022, 8:38 p.m. UTC | #3 
On 8/31/22 13:39, Vineet Gupta wrote:
> 
> 
> On 8/31/22 07:57, Palmer Dabbelt wrote:
>>>    if (flag_pic)
>>> -    riscv_cmodel = CM_PIC;
>>> +    riscv_cmodel = CM_MEDANY;
>>>
>>>    /* We get better code with explicit relocs for CM_MEDLOW, but
>>>       worse code for the others (for now).  Pick the best default.  */
>>
>> I'm fine either way on this one: having CM_PIC gone makes it a bit 
>> more likely to confuse CM_MEDANY with PIC, but flag_pic is overriding 
>> riscv_cmodel anyway so this isn't really used and deleting code is 
>> always a plus.
> 
> Indeed this was the most contentious part of removing CM_PIC, but it 
> seems this is the way fwd. I'll add Kito's comment from [1] in code to 
> make it more explicit.
> 
> [1]https://github.com/riscv-non-isa/riscv-c-api-doc/pull/11#issuecomment-686385585 

I think I'll punt on this one, in the short-term. The reason being it 
affects USE_LOAD_ADDRESS_MACRO.

#define USE_LOAD_ADDRESS_MACRO(sym)				\
   (!TARGET_EXPLICIT_RELOCS &&					\
    ((flag_pic							\
      && ((SYMBOL_REF_P (sym) && SYMBOL_REF_LOCAL_P (sym))	\
	 || ((GET_CODE (sym) == CONST)				\
	     && SYMBOL_REF_P (XEXP (XEXP (sym, 0),0))		\
	     && SYMBOL_REF_LOCAL_P (XEXP (XEXP (sym, 0),0)))))	\
      || riscv_cmodel == CM_MEDANY))

With the patch, PIC implies CM_MEDANY and thus will change codegen for 
pic non-local symbols to also use the load address macro. I think we 
want to go in the opposite direction, i.e. wean away from the asm macros 
and have gcc codegen natively. It seems there are bugs in that area so 
once we flush them out (after creating a few as I don't know of any 
existing documented ones) this will get cleaned out.

Thx
-Vineet
diff mbox series

Patch

diff --git a/gcc/config/riscv/riscv-c.cc b/gcc/config/riscv/riscv-c.cc
index bba72cf77a82..7064fcf142fe 100644
--- a/gcc/config/riscv/riscv-c.cc
+++ b/gcc/config/riscv/riscv-c.cc
@@ -92,13 +92,9 @@  riscv_cpu_cpp_builtins (cpp_reader *pfile)
       builtin_define ("__riscv_cmodel_medlow");
       break;
 
-    case CM_PIC:
-      /* FALLTHROUGH. */
-
     case CM_MEDANY:
       builtin_define ("__riscv_cmodel_medany");
       break;
-
     }
 
   if (TARGET_MIN_VLEN != 0)
diff --git a/gcc/config/riscv/riscv-opts.h b/gcc/config/riscv/riscv-opts.h
index 85e869e62e3a..ce3237beca7a 100644
--- a/gcc/config/riscv/riscv-opts.h
+++ b/gcc/config/riscv/riscv-opts.h
@@ -34,8 +34,7 @@  extern enum riscv_abi_type riscv_abi;
 
 enum riscv_code_model {
   CM_MEDLOW,
-  CM_MEDANY,
-  CM_PIC
+  CM_MEDANY
 };
 extern enum riscv_code_model riscv_cmodel;
 
diff --git a/gcc/config/riscv/riscv.cc b/gcc/config/riscv/riscv.cc
index 7c120eaa8e33..a239fe43047c 100644
--- a/gcc/config/riscv/riscv.cc
+++ b/gcc/config/riscv/riscv.cc
@@ -5162,7 +5162,7 @@  riscv_option_override (void)
   init_machine_status = &riscv_init_machine_status;
 
   if (flag_pic)
-    riscv_cmodel = CM_PIC;
+    riscv_cmodel = CM_MEDANY;
 
   /* We get better code with explicit relocs for CM_MEDLOW, but
      worse code for the others (for now).  Pick the best default.  */