On 4/22/21 3:25 PM, Andres Freund wrote:
Hi,
On 2021-04-22 09:35:48 -0700, Tom Stellard wrote:
On 4/21/21 6:40 AM, Honza Horak wrote:
I wrote a new patch based on the bug discussion[1]. It works around
the issue specifically on s390x rather than disabling specific
CPUs and features for all targets. The patch is attached.
Cool, this is a pretty clear improvement. There's a few minor things I'd
change to fit it into PG - do you mind if I send that to the thread at
[1] for you to test before I push it?
Sure, no problem.
+/*
+ * For the systemz target, LLVM uses a different datalayout for z13 and newer
+ * CPUs than it does for older CPUs. This can cause a mismatch in datalayouts
+ * in the case where the llvm_types_module is compiled with a pre-z13 CPU
+ * and the JIT is running on z13 or newer.
+ * See computeDataLayout() function in
+ * llvm/lib/Target/SystemZ/SystemZTargetMachine.cpp for information on the
+ * datalayout differences.
+ */
+static bool
+needs_systemz_workaround(void)
+{
+ bool ret = false;
+ LLVMContextRef llvm_context;
+ LLVMTypeRef vec_type;
+ LLVMTargetDataRef llvm_layoutref;
+ if (strncmp(LLVMGetTargetName(llvm_targetref), "systemz",
strlen("systemz")))
+ {
+ return false;
+ }
+
+ llvm_context = LLVMGetModuleContext(llvm_types_module);
+ vec_type = LLVMVectorType(LLVMIntTypeInContext(llvm_context, 32), 4);
+ llvm_layoutref = LLVMCreateTargetData(llvm_layout);
+ ret = (LLVMABIAlignmentOfType(llvm_layoutref, vec_type) == 16);
+ LLVMDisposeTargetData(llvm_layoutref);
+ return ret;
+}
I wonder if it'd be better to compare LLVMCopyStringRepOfTargetData() of
the llvm_types_module with the one of the JIT target machine, and only
specify -vector in that case? We currently support older LLVM versions
than the one that introduced the vector specific handling for systemz,
and I don't know what'd happen if we unnecessarily specified -vector.
The problem is that you have to pass the features to LLVMCreateTargetMachine
in order to know what the data layout of the JIT target is going to be,
so the only way to make this work, would be to create the TargetMachine
with the default features, check it's datalayout, and then re-create the
TargetMachine in order to apply the workaround. Maybe that's not so bad?
The other question I had is should we #ifdef ARCH_S390x in
needs_sytemz_workaround(), so we don't need to check the target
name.
-Tom
Greetings,
Andres Freund
