Add tests to reproduce pointer/index width confusion crashes

[krzysz00]

Summary

Some calls to GEPOperator::accumulateConstantOffset(APInt) passed the
pointer bitwidth as the width of the APInt, while the function asserts
that the width of its argument is equal to the index width of the GEP
pointer input. These values are almost always the same, so mixing up
which call to use doesn't usually cause issues. However, when dealing
with data layouts where these values are different, the passes tested
here can crash.

This will be fixed in D143437 .

[krzysz00]

Switch an XFAIL to a not --crash

[chapuni]

Do they really crash in -Asserts?

[dyung]

In D144673#4149028, @chapuni wrote:

Do they really crash in -Asserts?

We are also seeing the test fail on our internal builds without asserts. Running the opt command this makes sense because the crash is due to an assertion failure. Please fix the test to work both with/without asserts or require asserts when running the test.

[krzysz00]

Will fix logical tomorrow (feel free to revert until then)

How do I test for tripping an assertion reliably here? The point of these
tests is to confirm that another patch makes the assertion failure go away.

[yozhu]

We also saw failures on these three tests in our internal rolling noassert build/test.

[nikic]

You would have to add REQUIRES: asserts.

Though really, you should just include these as passing tests in the patch that fixes them. There is no pre-commit requirement for tests that crash.

[gribozavr2]

Sorry everyone, I reverted this change in https://github.com/llvm/llvm-project/commit/736e788c58f946f236bd5c80e17eb4a5448056d5.

Will fix logical tomorrow (feel free to revert until then)

I'd like to ask you to revert yourself immediately next time you're notified that a change broke builds. See the policy at https://llvm.org/docs/DeveloperPolicy.html#patch-reversion-policy

When should you revert your own change?

Any time you learn of a serious problem with a change, you should revert it. We strongly encourage “revert to green” as opposed to “fixing forward”.

There are many people in the community who depend on the health of the codebase. If everyone was leaving the code broken until they have time to commit a proper fix, the builds would be permanently broken.

[krzysz00]

Re filing my own reverts: I agree, and I know that it's reasonable to revert my own breaking changes so as to not break other people.

In this specific circumstance, the first I learned of the breaking was around my 1 AM when I saw y'all's comments about how the tests were failing with assertions off, and I wasn't in a position where I could do the revert at that time. So, rather than having everyone wait on me getting in to work the next day, I commented that it'd be fine to revert this.

On the subject of precommit tests, in the underlying diff, @arichardson recommend precommitting not --crash tests, so I went for that without realizing the impact on assertions-off builds.

Apologies for all the trouble.

llvm/test/Transforms/LowerTypeTests/distinct-index-width-crash.ll

+; RUN: not --crash opt -passes=lowertypetests %s -disable-output
+target datalayout = "e-p:64:64:64:32"
+
+@a = constant i32 1, !type !0
+@b = constant [2 x i32] [i32 2, i32 3], !type !1
+
+!0 = !{i32 0, !"typeid1"}
+!1 = !{i32 4, !"typeid1"}
+
+declare i1 @llvm.type.test(ptr %ptr, metadata %bitset) nounwind readnone
+
+; CHECK: @bar(
+define i1 @bar() {
+  ; CHECK: ret i1 true
+  %x = call i1 @llvm.type.test(ptr getelementptr ([2 x i32], ptr @b, i32 0, i32 1), metadata !"typeid1")
+  ret i1 %x
+}

llvm/test/Transforms/MergeFunc/different-index-width-gep-crash.ll

+; RUN: not --crash opt -disable-output -passes=mergefunc -S < %s
+
+target datalayout = "e-p:64:64-p2:128:128:128:32"
+
+define void @foo(ptr addrspace(2) %x) {
+    %tmp = getelementptr i32, ptr addrspace(2) %x, i32 1
+    ret void
+}
+
+define void @bar(ptr addrspace(2) %x) {
+    %tmp = getelementptr i32, ptr addrspace(2) %x, i32 1
+    ret void
+}

llvm/test/Transforms/MergeICmps/X86/distinct-index-width-crash.ll

+; RUN: not --crash opt < %s -passes=mergeicmps -verify-dom-info -disable-output
+
+target triple = "x86_64"
+
+; This is very much not an x86 ABI, in current use, but we're testing
+; that we've fixed a bug where accumulateConstantOffset() was called incorrectly.
+target datalayout = "e-p:64:64:64:32"
+
+; Define a cunstom data layout that has index width < pointer width
+; and make sure that doesn't mreak anything
+define void @fat_ptrs(ptr dereferenceable(16) %a, ptr dereferenceable(16) %b) {
+bb0:
+  %ptr_a1 = getelementptr inbounds [2 x i64], ptr %a, i64 0, i64 1
+  %ptr_b1 = getelementptr inbounds [2 x i64], ptr %b, i64 0, i64 1
+  br label %bb1
+
+bb1:                                              ; preds = %bb0
+  %a0 = load i64, ptr %a
+  %b0 = load i64, ptr %b
+  %cond0 = icmp eq i64 %a0, %b0
+  br i1 %cond0, label %bb2, label %bb3
+
+bb2:                                              ; preds = %bb1
+  %a1 = load i64, ptr %ptr_a1
+  %b1 = load i64, ptr %ptr_b1
+  %cond1 = icmp eq i64 %a1, %b1
+  br label %bb3
+
+bb3:                                              ; preds = %bb2, %bb1
+  %necessary = phi i1 [ %cond1, %bb2 ], [ false, %bb1 ]
+  ret void
+}