[PATCH] D118216: [RISCV] LUI used for address computation should not isAsCheapAsAMove

[Haocong Lu via Phabricator via llvm-commits]

Luhaocong added a comment.



In D118216#3316828 <https://reviews.llvm.org/D118216#3316828>, @jrtc27 wrote:

> In D118216#3316773 <https://reviews.llvm.org/D118216#3316773>, @benshi001 wrote:
>
>> In D118216#3316752 <https://reviews.llvm.org/D118216#3316752>, @jrtc27 wrote:
>>
>>> I objected and still believe this patch is fundamentally wrong. The problem needs solving elsewhere, not like this. Please revert.
>>
>> Although this solution is far from perfect, it does improve code quality. Could you please show a test case that gets wrong or worse assembly by this patch?
>> I suggest we can keep it and go on searching better solutions.
>
> I don't know, but I don't really care, it is blatantly wrong to say `LUI rd, <imm>` is not as cheap as a move, especially so to say `LUI rd, %hi(x)` isn't but `LUI rd, x` is, it's complete nonsense. There are lots of things you can commit that would improve codegen quality but are totally wrong and would get backed out immediately.

Here is a case to show the "wrong" you said, it does exist. 's1' spill unexpected due to the inaccurate cost evaluation of  `lui s1, %hi(g)`
Although this patch achieved greater codegen in most cases, it is really important to accurately describe the cost of instructions. 
Thanks for your review, I will revert it and look for more reasonable way.

  void func1(void);
  void func2(void);
  
  int g;
  
  int foo(int a) {
    int ret = 0;
    ret += g;
    if (a > 0)
      func1();
    else
      func2();
    ret += g;
    return ret;
  }



  foo:
          addi    sp, sp, -32
          sd      ra, 24(sp)
          sd      s0, 16(sp)
          sd      s1, 8(sp)
          lui     s1, %hi(g)
          lw      s0, %lo(g)(s1)
          blez    a0, .LBB0_2
          call    func1
          j       .LBB0_3
  .LBB0_2:
          call    func2
  .LBB0_3:
          lw      a0, %lo(g)(s1)
          addw    a0, a0, s0
          ld      ra, 24(sp)
          ld      s0, 16(sp)
          ld      s1, 8(sp)
          addi    sp, sp, 32
          ret




Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D118216