Re: [PATCH V2] riscv: generate builtin macro for compilation with strict alignment:

[Edwin Lu <ewlu@rivosinc.com>]

Hi Vineet,

On 8/8/2023 2:02 PM, Vineet Gupta wrote:
> 
> Maybe add a comment that in absence of -m[no-]strict-align, we use the 
> cpu tune param -> slow_unaligned_access and that default mcpu is rocket 
> which has that set to _slow.
> 

That sounds good to me!

>> +#if defined(__riscv_unaligned_avoid)
>> +#error "__riscv_unaligned_avoid is unexpectedly set"
>> +#endif
> 
> Lets first check what is really expected.
> #if !defined (_slow) #error
> 
>> +
>> +/* Check __riscv_unaligned_slow xor __riscv_unaligned_fast is set.  */
>> +#if !defined(__riscv_unaligned_slow) && !defined(__riscv_unaligned_fast)
>> +#error "either __riscv_unaligned_slow or__riscv_unaligned_fast is not 
>> set"
>> +#endif
>> +
>> +#if defined(__riscv_unaligned_slow) && defined(__riscv_unaligned_fast)
>> +#error "both __riscv_unaligned_slow and__riscv_unaligned_fast are set"
>> +#endif
>> +
> 
> I think we can simplify this a bit (sorry I if wasn't clear enough in 
> our off-list discussions).
> We now need to ensure that unexpected toggles are NOT defined: #if 
> defined(_avoid) || defined(_fast) #error
> I don't think we need to check for the combinations as that is covered 
> by first one and this.
> 
> While separate #error prints for 2 failures are ideal, personally it 
> feels excessive given that the current implementation will only ever 
> generate 1 of them. If a future code change breaks that assumption, the 
> onus is on that change to fix/update the errors.
> 

That makes sense to me. I was thinking that adding some more checks 
would help clarify the assumptions but it did just make things overly 
verbose.

> 
> Same as my comment for attribute-1. Please recheck all of them.
> 

Thanks for the feedback! I will be sure to update all of them in the 
next iteration.

Edwin