[PATCH] D150862: [RISCV][CodeGenPrepare] Select the optimal base offset for GEPs with large offset

[Yingwei Zheng via Phabricator via llvm-commits]

dtcxzyw marked 2 inline comments as done.
dtcxzyw added a comment.

In D150862#4359123 <https://reviews.llvm.org/D150862#4359123>, @StephenFan wrote:

> I think we still resolved part of this problem instead of all of it. The reason is for
>
>   define void @foo(ptr %dest) {
>     %p1 = getelementptr i8, ptr %dest, i32 2048
>     store i8 1, ptr %p1
>     %p2 = getelementptr i8, ptr %dest, i32 2049
>     store i8 1, ptr %p2
>     %p3 = getelementptr i8, ptr %dest, i32 2050
>     store i8 1, ptr %p3
>     %p4 = getelementptr i8, ptr %dest, i32 2051
>     store i8 1, ptr %p4
>     ret void
>   }
>
> the extra `addi	a1, a0, 1` is produced by `2048` can not be represented in Int<12>. And this patch just resolved this kind of case. Consider that if the offset is `4194305`, the `addi` still exists since the instruction selector selects it to
>
>   lui	a1, 1024
>   addiw	a1, a1, 1
>
> If you want to solve all of these kinds of cases, you need to repeat all the logic that selecting a large constant. So maybe handle it after instruction selection is a better way?

This case has been fixed by choosing an offset divisible by 4096. If we handle it after ISel, we will retrieve the whole 'add-imm' chain for each load/store instruction. As @craig.topper commented in D150665 <https://reviews.llvm.org/D150665>, it defeats the point of the common base optimization.



================
Comment at: llvm/lib/Target/RISCV/RISCVISelLowering.cpp:16341
+  // Infer type of memory ops from the result element type of GEPs to use
+  // compressed load/store insts.
+  if (Subtarget.hasStdExtCOrZca()) {
----------------
luke wrote:
> Just a heads up, using the source/result element types of GEPs to infer the type of memory access is not always accurate, I've encountered this myself in https://reviews.llvm.org/D149889
> 
> It's also mentioned in CodeGenPrepare.cpp:
> ```
>           // The result type of the GEP might not be the type of the memory
>           // access.
> ```
> 
> I don't think this should block this patch, I just wanted to point it out. The code here is consistent with how CodeGenPrepare is inferring it.
Thank you for your suggestion. I will fix this after D150583 is ready.



Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D150862