[PATCH] D148129: [GVN] Adjust metadata for coerced load CSE

[Nikita Popov via Phabricator via llvm-commits]

nikic created this revision.
nikic added reviewers: StephenFan, jdoerfert, efriedma, nlopes.
Herald added subscribers: hiraditya, Prazek.
Herald added a project: All.
nikic requested review of this revision.
Herald added a project: LLVM.
Herald added a subscriber: llvm-commits.

When reusing a load in a way that requires coercion (i.e. casts or bit extraction) we currently fail to adjust metadata. Unfortunately, none of our existing tooling for this is really suitable, because combineMetadataForCSE() expects both loads to have the same type. In this case we may work on loads of different types and possibly and offset memory location.

As such, what this patch does is to simply drop all metadata, with the following exceptions:

- invariant.load/invariant.group metadata is retained, because violation always causes UB (and we get test regressions otherwise...)
- If the load is !noundef, keep all metadata, as this will turn poison-generating metadata into UB as well.

This fixes the miscompile that was exposed by D146629 <https://reviews.llvm.org/D146629>.


https://reviews.llvm.org/D148129

Files:
  llvm/lib/Transforms/Scalar/GVN.cpp
  llvm/test/Transforms/GVN/metadata.ll


Index: llvm/test/Transforms/GVN/metadata.ll
===================================================================
--- llvm/test/Transforms/GVN/metadata.ll
+++ llvm/test/Transforms/GVN/metadata.ll
@@ -137,7 +137,7 @@
 define void @load_ptr_nonnull_to_i64(ptr %p) {
 ; CHECK-LABEL: define void @load_ptr_nonnull_to_i64
 ; CHECK-SAME: (ptr [[P:%.*]]) {
-; CHECK-NEXT:    [[VAL:%.*]] = load ptr, ptr [[P]], align 8, !nonnull !6
+; CHECK-NEXT:    [[VAL:%.*]] = load ptr, ptr [[P]], align 8
 ; CHECK-NEXT:    [[VAL_INT:%.*]] = ptrtoint ptr [[VAL]] to i64
 ; CHECK-NEXT:    call void @use.i64(i64 [[VAL_INT]])
 ; CHECK-NEXT:    call void @use.i64(i64 [[VAL_INT]])
@@ -188,7 +188,7 @@
 define void @load_ptr_nonnull_to_i32(ptr %p) {
 ; CHECK-LABEL: define void @load_ptr_nonnull_to_i32
 ; CHECK-SAME: (ptr [[P:%.*]]) {
-; CHECK-NEXT:    [[VAL:%.*]] = load ptr, ptr [[P]], align 8, !nonnull !6
+; CHECK-NEXT:    [[VAL:%.*]] = load ptr, ptr [[P]], align 8
 ; CHECK-NEXT:    [[VAL_INT:%.*]] = ptrtoint ptr [[VAL]] to i64
 ; CHECK-NEXT:    [[TMP1:%.*]] = trunc i64 [[VAL_INT]] to i32
 ; CHECK-NEXT:    call void @use.i64(i64 [[VAL_INT]])
@@ -206,7 +206,7 @@
 define void @load_i64_range_to_i32_range(ptr %p) {
 ; CHECK-LABEL: define void @load_i64_range_to_i32_range
 ; CHECK-SAME: (ptr [[P:%.*]]) {
-; CHECK-NEXT:    [[VAL:%.*]] = load i64, ptr [[P]], align 8, !range [[RNG7:![0-9]+]]
+; CHECK-NEXT:    [[VAL:%.*]] = load i64, ptr [[P]], align 8
 ; CHECK-NEXT:    [[TMP1:%.*]] = trunc i64 [[VAL]] to i32
 ; CHECK-NEXT:    call void @use.i64(i64 [[VAL]])
 ; CHECK-NEXT:    call void @use.i32(i32 [[TMP1]])
@@ -239,5 +239,4 @@
 ; CHECK: [[RNG4]] = !{i32 10, i32 1}
 ; CHECK: [[RNG5]] = !{i32 3, i32 4, i32 5, i32 2}
 ; CHECK: [[META6:![0-9]+]] = !{}
-; CHECK: [[RNG7]] = !{i64 0, i64 10}
 ;.
Index: llvm/lib/Transforms/Scalar/GVN.cpp
===================================================================
--- llvm/lib/Transforms/Scalar/GVN.cpp
+++ llvm/lib/Transforms/Scalar/GVN.cpp
@@ -1005,6 +1005,16 @@
       // but then there all of the operations based on it would need to be
       // rehashed.  Just leave the dead load around.
       gvn.getMemDep().removeInstruction(CoercedLoad);
+      // We are adding a new user for this load, for which the original
+      // metadata may not hold. Additionally, the new load may have a different
+      // size and type, so their metadata cannot be combined in any
+      // straightforward way.
+      // Drop all metadata that is not known to cause immediate UB on violation,
+      // unless the load has !noundef, in which case all metadata violations
+      // will be promoted to UB.
+      if (!CoercedLoad->hasMetadata(LLVMContext::MD_noundef))
+        CoercedLoad->dropUnknownNonDebugMetadata(
+            {LLVMContext::MD_invariant_load, LLVMContext::MD_invariant_group});
       LLVM_DEBUG(dbgs() << "GVN COERCED NONLOCAL LOAD:\nOffset: " << Offset
                         << "  " << *getCoercedLoadValue() << '\n'
                         << *Res << '\n'


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D148129.512816.patch
Type: text/x-patch
Size: 3001 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20230412/19b20304/attachment.bin>