[PATCH] D149396: [NFC] Add a test case to make sure !prof is preserved when one instruction CSE'ed another.

Fri Apr 28 09:57:19 PDT 2023

mingmingl added a comment.

In D149396#4303857 <https://reviews.llvm.org/D149396#4303857>, @tejohnson wrote:

> Confused - there was a code change here previously - is it no longer needed?

Yes the code change is no longer needed. I initially thought `K`'s metadata needs to be explicitly reset to be preserved in combineMetadata <https://github.com/llvm/llvm-project/blob/ade3c6a6a88ed3a9b06c076406f196da9d3cc1b9/llvm/lib/Transforms/Utils/Local.cpp#L2636>  when `KDominatesJ` is false (or `DoesKMove` is false), but realized it shouldn't be the case, so reverted the code change (to explicitly reset metadata again)

In D149396#4304082 <https://reviews.llvm.org/D149396#4304082>, @davidxl wrote:

> looks like the wrong test case is uploaded? Should it be CSE of direct calls with "branch_weight" profile instead of indirect calls (indirect calls are unlikely to be CSEed without points to information).

Here the indirect call result CSE'ed a load-after-store instruction, rather than indirect call instruction itself is eliminated.

================
Comment at: llvm/test/Transforms/EarlyCSE/metadata.ll:21
+  %0 = load ptr, ptr %vfn, align 8
+  %call1 = tail call i32 %0(ptr %call, i32 %a, i32 %b), !prof !0
+  store i32 %call1, ptr %x
----------------
tejohnson wrote:
> What was being CSE'd here that caused it to get dropped?
Here the optimizer finds `%foo` should have the same value as `%call1` so it CSE'ed the load operation, and combines metadata of ` %call1 = tail call i32 %0(ptr %call, i32 %a, i32 %b), !prof !0` and  `%foo = load i32, ptr %x, align 4`.

Before D148877,  `!prof` is not added to known-id-set, and turns out unknown types of metadata are dropped in the [[ https://github.com/llvm/llvm-project/blob/ade3c6a6a88ed3a9b06c076406f196da9d3cc1b9/llvm/lib/Transforms/Utils/Local.cpp#L2639 | implementation ]].

This test is mainly added to make sure there won't be regressions for this kind of pattern. The pattern is observed it in application code; looks like the result of indirect call is used as function arguments initially; after the function is inlined load-after-store is exposed (I didn't try to get a c++ repro since the IR is a little complex)

With 'opt -passes=early-cse -debug metadata.ll' debug log gives 
`EarlyCSE CSE LOAD:   %foo = load i32, ptr %x, align 4  to:   store i32 %call1, ptr %x, align 4` ([[ https://github.com/llvm/llvm-project/blob/8c8fe11916b03887f89f04b216e0044f2a9cb505/llvm/lib/Transforms/Scalar/EarlyCSE.cpp#L1489 | related source code ]])

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

https://reviews.llvm.org/D149396