[PATCH] D101875: [RISCV] Prefer to lower MC_GlobalAddress operands to .Lfoo$local

Fangrui Song via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue May 11 11:25:27 PDT 2021


MaskRay added a comment.

In D101875#2751502 <https://reviews.llvm.org/D101875#2751502>, @jrtc27 wrote:

> In D101875#2751459 <https://reviews.llvm.org/D101875#2751459>, @MaskRay wrote:
>
>> In D101875#2751448 <https://reviews.llvm.org/D101875#2751448>, @jrtc27 wrote:
>>
>>> In D101875#2751443 <https://reviews.llvm.org/D101875#2751443>, @MaskRay wrote:
>>>
>>>> In D101875#2751397 <https://reviews.llvm.org/D101875#2751397>, @jrtc27 wrote:
>>>>
>>>>> Ok, but you _still_ haven't wrapped the RUN lines like the example I posted
>>>>
>>>> Not sure this will improve readability. The longest RUN line just takes 99 characters. We don't enforce 80-character rule for tests.
>>>
>>> We enforce this uniform formatting in the RISC-V tests.
>>
>> Quite a few `rvv/` tests have 100+-character RUN lines.
>
> Yes, rvv/ tests in both CodeGen and MC have ended up diverging because I wasn't reviewing them as vectors are not something I care much about except when they interact with memory.
>
>> (There is a style disagreement on where `|` is inserted. In many tests `|` is placed at the end of the previous line while here `|` is placed at the beginning of the continuation line)
>
> All the ones directly under llvm/test/CodeGen/RISCV have the `|` on the next line. None of them have a single-line llc RUN line.
>
> The only inconsistency is whether there are 1+2 spaces before the `|` (431 cases) or 1+4 spaces (32 cases). I guess pic-models.ll happens to be one of the odd ones out.

I picked +2 spaces, just because it is more common.

If you don't mind, I'd still want to switch to 2 spaces since it is more common elsewhere.

(If a new thing adds +4 spaces and I was a reviewer, I'd definitely raise the point before they go in.)

>>> Even though the line is short, splitting it makes it much easier to compare each RUN line.
>>
>> To me it's different. Without interleaving wrapped `FileCheck` lines, it is easier to compare what options differ between two consecutive lines.
>
> We can debate personal preferences all we like, but that doesn't change the fact that there is a consistent style in the directory that this is going against.




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D101875



More information about the llvm-commits mailing list