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

[Vineet Gupta <vineetg@rivosinc.com>]

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