[PATCH] D45748: [RISCV] Separate base from offset in lowerGlobalAddress

[Alex Bradbury via Phabricator via llvm-commits]

asb added a comment.
Herald added a subscriber: edward-jones.

Thanks Sameer. This is definitely a win for a range of inputs, but I'm also seeing regressions. An extra instruction is now required for any standalone global access + offset. There are even some cases where the global access is going from 2 instructions to 4 instructions (global with very large offset, requiring lui+addi). Do you have any thoughts on addressing these cases?

The CHECK lines for the new test file are best generated with update_llc_test_checks.py, as that makes it easy to regenerate in the future and normalises whitespace. I'm also seeing test changes for other in-tree tests, which also need regenerating in this patch. I always prefer to have any attributes that aren't necessary to the function of the test removed, so the test file might end up looking like:

  ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
  ; RUN: llc -mtriple=riscv32  < %s | FileCheck  %s
  
  ; This test case checks that base for a gloabl address will be reused between
  ; different accesses. This will allow for a shorter instruction sequence
  ; for access.
  ; lui     a0, %hi(s+44)
  ; addi    a1, zero, 20
  ; sw      a1, %lo(s+44)(a0)
  
  ; lui     a0, %hi(s+8)
  ; addi    a1, zero, 10
  ; sw      a1, %lo(s+8)(a0)
  ; The first function shows the case when the offset is small enough to be
  ; folded, the second is when the offset is too big.
  
  ; TODO: for the big offset, the addi following the lui for the offset
  ;       can be optimized away by folding the %lo into the immediate part
  ;       of the sw.
  
  %struct.S = type { i32, i32, %struct.S_nest, i32, i32, %struct.S_nest, i32, i32, %struct.S_nest }
  %struct.S_nest = type { i32, i32, i32, i32, i32, i32 }
  
  %struct.S2 = type { i32, [4100 x i32], i32, [4100 x i32], i32 }
  
  @s = global %struct.S zeroinitializer, align 4
  @s2 = global %struct.S2 zeroinitializer, align 4
  
  define void @foo() nounwind {
  ; CHECK-LABEL: foo:
  ; CHECK:       # %bb.0: # %entry
  ; CHECK-NEXT:    lui a0, %hi(s)
  ; CHECK-NEXT:    addi a0, a0, %lo(s)
  ; CHECK-NEXT:    addi a1, zero, 20
  ; CHECK-NEXT:    sw a1, 44(a0)
  ; CHECK-NEXT:    addi a1, zero, 10
  ; CHECK-NEXT:    sw a1, 8(a0)
  ; CHECK-NEXT:    addi a1, zero, 30
  ; CHECK-NEXT:    sw a1, 80(a0)
  ; CHECK-NEXT:    ret
  entry:
    store i32 10, i32* getelementptr inbounds (%struct.S, %struct.S* @s, i32 0, i32 2, i32 0), align 4
    store i32 20, i32* getelementptr inbounds (%struct.S, %struct.S* @s, i32 0, i32 5, i32 1), align 4
    store i32 30, i32* getelementptr inbounds (%struct.S, %struct.S* @s, i32 0, i32 8, i32 2), align 4
    ret void
  }
  
  define void @bar() nounwind {
  ; CHECK-LABEL: bar:
  ; CHECK:       # %bb.0: # %entry
  ; CHECK-NEXT:    lui a0, 8
  ; CHECK-NEXT:    addi a0, a0, 40
  ; CHECK-NEXT:    lui a1, %hi(s2)
  ; CHECK-NEXT:    addi a1, a1, %lo(s2)
  ; CHECK-NEXT:    add a0, a1, a0
  ; CHECK-NEXT:    addi a2, zero, 50
  ; CHECK-NEXT:    sw a2, 0(a0)
  ; CHECK-NEXT:    lui a0, 4
  ; CHECK-NEXT:    addi a0, a0, 20
  ; CHECK-NEXT:    add a0, a1, a0
  ; CHECK-NEXT:    addi a1, zero, 40
  ; CHECK-NEXT:    sw a1, 0(a0)
  ; CHECK-NEXT:    ret
  entry:
    store i32 40, i32* getelementptr inbounds (%struct.S2, %struct.S2* @s2, i32 0, i32 2), align 4
    store i32 50, i32* getelementptr inbounds (%struct.S2, %struct.S2* @s2, i32 0, i32 4), align 4
    ret void
  }



================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:221
 
 SDValue RISCVTargetLowering::lowerGlobalAddress(SDValue Op,
                                                 SelectionDAG &DAG) const {
----------------
There are a few clang-tidy issues in this function (extra space after return, lack of space before RISCVII::MO_HI.


================
Comment at: lib/Target/RISCV/RISCVISelLowering.cpp:232
     report_fatal_error("Unable to lowerGlobalAddress");
 
   SDValue GAHi =
----------------
It would be good to add a comment here, something along the lines of:

// Lower the base global address and the offset separately, allowing the base address to be reused across different accesses.


================
Comment at: test/CodeGen/RISCV/hoist-global-addr-base.ll:1
+; RUN: llc -mtriple=riscv32  < %s | FileCheck  %s
+
----------------
Please generate the CHECK lines with update_llc_test_checks.py.


https://reviews.llvm.org/D45748