[PATCH] D127392: [AggressiveInstCombine] Combine consecutive loads which are being merged to form a wider load.

[Sanjay Patel via Phabricator via llvm-commits]

spatel added a comment.

How does this code account for potential memory accesses between the loads that are getting combined?

  define i16 @loadCombine_2consecutive_store_between(ptr %p) {
    %p1 = getelementptr i8, ptr %p, i32 1
    %l1 = load i8, ptr %p, align 2
    store i8 42, ptr %p  ; this must happen after a combined load?
    %l2 = load i8, ptr %p1
  
    %e1 = zext i8 %l1 to i16
    %e2 = zext i8 %l2 to i16
    %s2 = shl i16 %e2, 8
    %o1 = or i16 %e1, %s2
    ret i16 %o1
  }



================
Comment at: llvm/test/Transforms/AggressiveInstCombine/or-load.ll:5
+
+define i16 @loadCombine_2consecutive(ptr %p) {
+; ALL-LABEL: @loadCombine_2consecutive(
----------------
We need to have this test duplicated on a target (x86?) where the fold actually happens. Target-specific tests will need to go inside target subdirectories, otherwise we'll break testing bots. We can pre-commit those once we have the right set of tests.


================
Comment at: llvm/test/Transforms/AggressiveInstCombine/or-load.ll:24
+
+  %s1 = shl i16 %e1, 0
+  %s2 = shl i16 %e2, 8
----------------
Why do the tests have non-canonical code (shift-by-0)? I don't think we'd ever see this pattern given where AIC is in the opt pipeline.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D127392