llvm: Fix array ABI test to not check equality implementation

[maurer]

LLVM has moved memcmp expansion in the pipeline, resulting in the bcmp call being expanded into loads and register comparisons, which breaks the test.

See llvm/llvm-project#77370

Based on history, I believe the test actually intended validate that these arrays were being passed as pointer arguments, which can be done more directly.

r? @durin42
cc @scottmcm (author of the test being modified)

@rustbot label llvm-main

[maurer]

Fixes buildkite

[scottmcm]

Based on history, I believe the test actually intended validate that these arrays were being passed as pointer arguments, which can be done more directly.

No, that's incorrect. If it was just that, it would be in the ABI test files instead.

It's from #85828 which is about how the equality codegen is generated. This test is there to test the case that's not using the special type_ix path, so I think it should still have validation for the body -- otherwise it's not useful.

View changes since this review

[rustbot]

Reminder, once the PR becomes ready for a review, use @rustbot ready.

[scottmcm]

Pondering: maybe this raw_eq override should just be removed on newer-llvm?

rust/compiler/rustc_codegen_llvm/src/intrinsic.rs

Lines 514 to 551 in 2972b5e

	sym::raw_eq => {
	use BackendRepr::*;
	let tp_ty = fn_args.type_at(0);
	let layout = self.layout_of(tp_ty).layout;
	let use_integer_compare = match layout.backend_repr() {
	Scalar(_) | ScalarPair(_, _) => true,
	SimdVector { .. } => false,
	SimdScalableVector { .. } => {
	tcx.dcx().emit_err(InvalidMonomorphization::NonScalableType {
	span,
	name: sym::raw_eq,
	ty: tp_ty,
	});
	return Ok(());
	}
	Memory { .. } => {
	// For rusty ABIs, small aggregates are actually passed
	// as `RegKind::Integer` (see `FnAbi::adjust_for_abi`),
	// so we re-use that same threshold here.
	layout.size() <= self.data_layout().pointer_size() * 2
	}
	};
	let a = args[0].immediate();
	let b = args[1].immediate();
	if layout.size().bytes() == 0 {
	self.const_bool(true)
	} else if use_integer_compare {
	let integer_ty = self.type_ix(layout.size().bits());
	let a_val = self.load(integer_ty, a, layout.align().abi);
	let b_val = self.load(integer_ty, b, layout.align().abi);
	self.icmp(IntPredicate::IntEQ, a_val, b_val)
	} else {
	let n = self.const_usize(layout.size().bytes());
	let cmp = self.call_intrinsic("memcmp", &[], &[a, b, n]);
	self.icmp(IntPredicate::IntEQ, cmp, self.const_int(self.type_int(), 0))
	}
	}

llvm/llvm-project#77370 ought to be strictly better than it. The LLVM-specific override was only added because the memcmp-to-load+icmp was only in the backend, so with it in the middle-end we should probably use LLVM's version of it, not my little hack.

(Assuming, of course, that we still do a good job on [u8;4] equality and such, but I figure it would.)

[maurer]

I'll try out switching off the raw_eq override on newer LLVM and see if the other tests pass, but it will still optimize into inlining the equality check on new LLVM. Suggestions on what I should do to that test case?

[maurer]

Specifically, the output looks like this:

         24: ; Function Attrs: mustprogress nofree norecurse nounwind nonlazybind willreturn memory(argmem: read) uwtable
         25: define noundef zeroext i1 @array_eq_value_still_passed_by_pointer(ptr noalias noundef readonly align 2 captures(none) dead_on_return dereferenceable(18) %a, ptr noalias noundef readonly align 2 captures(none) dead_on_return dereferenceable(18) %b) unnamed_addr #2 {
         26: start:
check:29           X error: no match found
         27:  %0 = load i128, ptr %a, align 2
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         28:  %1 = load i128, ptr %b, align 2
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         29:  %2 = xor i128 %0, %1
check:29     ~~~~~~~~~~~~~~~~~~~~~~
         30:  %3 = getelementptr i8, ptr %a, i64 16
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         31:  %4 = getelementptr i8, ptr %b, i64 16
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         32:  %5 = load i16, ptr %3, align 2
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         33:  %6 = load i16, ptr %4, align 2
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
         34:  %7 = zext i16 %5 to i128
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~
         35:  %8 = zext i16 %6 to i128
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~
         36:  %9 = xor i128 %7, %8
check:29     ~~~~~~~~~~~~~~~~~~~~~~
         37:  %10 = or i128 %2, %9
check:29     ~~~~~~~~~~~~~~~~~~~~~~
         38:  %11 = icmp ne i128 %10, 0
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~~
         39:  %12 = zext i1 %11 to i32
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~
         40:  %13 = icmp eq i32 %12, 0
check:29     ~~~~~~~~~~~~~~~~~~~~~~~~~~
         41:  ret i1 %13
check:29     ~~~~~~~~~~~~
         42: }

[durin42]

r? @scottmcm

[scottmcm]

Using revisions for different checks for different LLVM versions would be the most straight-forward way to check different stuff. Maybe validating that there's are icmps but no allocas nor calls would suffice? Checking the exact expansion probably isn't something we need.

---

As an aside, seems like there's an LLVM bug to be seen in that output too, perhaps?

         39:  %12 = zext i1 %11 to i32
         40:  %13 = icmp eq i32 %12, 0

in opt-level=3 is weird to me -- why not %13 = xor i1 %11, true? Or, better, inverting the checks earlier to produce the correct thing more directly and not the the BitNot.

[maurer]

I tried suppressing the rewrite in the sense of making use_integer_compare be automatically false on newer LLVMs, and it produces significantly worse IR:

; Function Attrs: mustprogress nofree norecurse nosync nounwind nonlazybind willreturn memory(none) uwtable                                                                           
define noundef zeroext i1 @array_eq_value(i48 noundef %0, i48 noundef %1) unnamed_addr #0 {                                                                                           
start:                                                                                                                                                                                
  %b = alloca [6 x i8], align 8                                                                                                                                                       
  %a = alloca [6 x i8], align 8                                                                                                                                                       
  store i48 %0, ptr %a, align 8                                                                                                                                                       
  store i48 %1, ptr %b, align 8                                                                                                                                                       
  %2 = load i32, ptr %a, align 8                                                                                                                                                      
  %3 = load i32, ptr %b, align 8                                                                                                                                                      
  %4 = xor i32 %2, %3                                                                                                                                                                 
  %5 = getelementptr i8, ptr %a, i64 4                                                                                                                                                
  %6 = getelementptr i8, ptr %b, i64 4                                                                                                                                                
  %7 = load i16, ptr %5, align 4                                                                                                                                                      
  %8 = load i16, ptr %6, align 4                                                                                                                                                      
  %9 = zext i16 %7 to i32                                                                                                                                                             
  %10 = zext i16 %8 to i32                                                                                                                                                            
  %11 = xor i32 %9, %10                                                                                                                                                               
  %12 = or i32 %4, %11                                                                                                                                                                
  %13 = icmp ne i32 %12, 0                                                                                                                                                            
  %14 = zext i1 %13 to i32                                                                                                                                                            
  %15 = icmp eq i32 %14, 0                                                                                                                                                            
  ret i1 %15                                                                                                                                                                          
}

The LLVM PR suggests that "we mostly don't benefit from additional optimizations yet", so perhaps just calling memcmp could be revisited later, but the current state of LLVM doesn't seem to produce reasonable IR with it.

I've updated the test as you suggested, splitting the test into a llvm-current and llvm-next revision, with llvm-current checking the current properties and llvm-next just validating that there are no alloca or call statements, and there is an icmp.

@rustbot ready

[scottmcm]

and it produces significantly worse IR:

Ah, thanks for trying it. Hopefully we'll be able to do that in future.

@bors r+ rollup=iffy (multi-version LLVM codegen test is extra scary)

[rust-bors]

📌 Commit d6be991 has been approved by scottmcm

It is now in the queue for this repository.