[CodeGen] Use TargetLowering for TypeInfo of PointerTy #93469

jplehr · 2024-05-27T12:54:53Z

This uses the TargetLowering getSimpleValueType mechanism to retrieve the ValueType info and work around a build issue we were seeing for the miniQMC application after
#92671.

llvmbot · 2024-05-27T12:55:25Z

@llvm/pr-subscribers-llvm-analysis
@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-backend-x86

Author: Jan Patrick Lehr (jplehr)

Changes

This uses the TargetLowering getSimpleValueType mechanism to retrieve the ValueType info and work around a build issue we were seeing for the miniQMC application after
#92671.

Full diff: https://github.com/llvm/llvm-project/pull/93469.diff

1 Files Affected:

(modified) llvm/lib/Target/X86/X86TargetTransformInfo.cpp (+2-1)

diff --git a/llvm/lib/Target/X86/X86TargetTransformInfo.cpp b/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
index ac66144aeaaec..6616fe9274a56 100644
--- a/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
+++ b/llvm/lib/Target/X86/X86TargetTransformInfo.cpp
@@ -6257,7 +6257,8 @@ InstructionCost X86TTIImpl::getInterleavedMemoryOpCostAVX512(
                                 AddressSpace, CostKind);
 
   unsigned VF = VecTy->getNumElements() / Factor;
-  MVT VT = MVT::getVectorVT(MVT::getVT(VecTy->getScalarType()), VF);
+  MVT VT =
+      MVT::getVectorVT(TLI->getSimpleValueType(DL, VecTy->getScalarType()), VF);
 
   InstructionCost MaskCost;
   if (UseMaskedMemOp) {

arsenm

Needs test

jplehr · 2024-05-27T13:55:30Z

I have to admit I'm a bit lost on what and where to add as a test. Guidance much appreciated!

jrtc27 · 2024-05-27T19:19:15Z

Depends when the crash occurs, but for example a minimal function that triggers the crash in placed in llvm/test/Transforms/SLPVectorizer/X86 would be reasonable if the crash occurred during the SLPVectorizer pass. See other files for examples.

jplehr · 2024-05-28T15:05:53Z

Thank you for your help, I hope I have moved this PR closer to being acceptable.

I have reduced the failing application into a small test case that reproduces the error. I have checked the loop-vectorizer output from before the PR that broke the build to the output that it produces now and they match.

My main concern (no more assertion hit) is already satisfied. However, should the generated code be added as the expected IR? Should the test be renamed? (edit: it probably should, and I'm open to suggestions)

Thank you.

arsenm · 2024-05-28T15:15:19Z

llvm/test/Transforms/LoopVectorize/cost-model-no-assert.ll

@@ -0,0 +1,31 @@
+; RUN: opt < %s -passes="loop-vectorize"


Don't need the quotes. Also should use file check for something on the output. Alternatively, since this was an assertion in the cost model, can you get this down to one operation following along with test/Analysis/CostModel tests?

I think you're right, the test should probably go to the cost model. I don't know if I can reduce it further.

arsenm · 2024-05-28T15:16:03Z

llvm/test/Transforms/LoopVectorize/cost-model-no-assert.ll

+; ModuleID = 'reduced.ll'
+source_filename = "reduced.ll"
+;target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-i128:128-f80:128-n8:16:32:64-S128"


Can drop this

arsenm · 2024-05-28T15:16:18Z

llvm/test/Transforms/LoopVectorize/cost-model-no-assert.ll

+target triple = "x86_64-unknown-linux-gnu"
+
+; Function Attrs: nofree norecurse nosync nounwind memory(read, argmem: readwrite, inaccessiblemem: none)
+define noalias noundef ptr @foo(ptr readonly %__first, ptr writeonly %__last) local_unnamed_addr #0 {


Should be able to drop noundef, local_unnamed_addr

arsenm · 2024-05-28T15:16:53Z

llvm/test/Transforms/LoopVectorize/cost-model-no-assert.ll

+  ret ptr null
+}
+
+attributes #0 = { nofree norecurse nosync nounwind memory(read, argmem: readwrite, inaccessiblemem: none) "target-cpu"="znver4" }


Probably don't need any of these, except znver4?

jrtc27 · 2024-05-28T18:58:54Z

The test case needs REQUIRES: asserts as -debug-only doesn't always exist (yes, assertions and debug a bit conflated...), and you can strip out a bunch of comments and attributes. Testing locally, the following still seems to trigger the same assertion:

diff --git a/llvm/test/Analysis/CostModel/X86/handle-iptr-with-data-layout-to-not-assert.ll b/llvm/test/Analysis/CostModel/X86/handle-iptr-with-data-layout-to-not-assert.ll
index 5278085b1182..d03dab7f7b14 100644
--- a/llvm/test/Analysis/CostModel/X86/handle-iptr-with-data-layout-to-not-assert.ll
+++ b/llvm/test/Analysis/CostModel/X86/handle-iptr-with-data-layout-to-not-assert.ll
@@ -1,9 +1,9 @@
 ; NOTE: Assertions have been autogenerated by utils/update_analyze_test_checks.py UTC_ARGS: --filter "LV: Found an estimated cost of [0-9] for VF [0-9] For instruction:\s*store ptr %[0-9], ptr %__last" --filter "LV: Found an estimated cost of [0-9] for VF [0-9] For instruction:\s*store ptr %[0-9]" --version 5
+; REQUIRES: asserts
 ; RUN: opt -passes=loop-vectorize -debug-only=loop-vectorize -S < %s 2>&1 | FileCheck %s --check-prefix=CHECK_IPTR
 target triple = "x86_64-unknown-linux-gnu"
 
-; Function Attrs: nofree norecurse nosync nounwind memory(read, argmem: readwrite, inaccessiblemem: none)
-define noalias ptr @foo(ptr readonly %__first, ptr writeonly %__last) #0 {
+define ptr @foo(ptr %__first, ptr %__last) #0 {
 ; CHECK_IPTR-LABEL: 'foo'
 ; CHECK_IPTR:  LV: Found an estimated cost of 1 for VF 1 For instruction: store ptr %0, ptr %__last, align 8
 ; CHECK_IPTR:  LV: Found an estimated cost of 2 for VF 2 For instruction: store ptr %0, ptr %__last, align 8
@@ -14,10 +14,10 @@ entry:
   %cmp.not1 = icmp eq ptr %__first, %__last
   br i1 %cmp.not1, label %for.end, label %for.body.preheader
 
-for.body.preheader:                               ; preds = %entry
+for.body.preheader:
   br label %for.body
 
-for.body:                                         ; preds = %for.body.preheader, %for.body
+for.body:
   %__first.addr.02 = phi ptr [ %incdec.ptr, %for.body ], [ %__first, %for.body.preheader ]
   %0 = load ptr, ptr %__first.addr.02, align 8
   store ptr %0, ptr %__last, align 8
@@ -25,10 +25,10 @@ for.body:                                         ; preds = %for.body.preheader,
   %cmp.not = icmp eq ptr %incdec.ptr, %__last
   br i1 %cmp.not, label %for.end.loopexit, label %for.body
 
-for.end.loopexit:                                 ; preds = %for.body
+for.end.loopexit:
   br label %for.end
 
-for.end:                                          ; preds = %for.end.loopexit, %entry
+for.end:
   ret ptr null
 }

Also, please use kebab-case rather than snake_case for your FileCheck prefix, though given there's just the one in use why not just leave it at the default CHECK?

This uses the TargetLowering getSimpleValueType mechanism to retrieve the ValueType info and work around a build issue we were seeing for the miniQMC application after llvm#92671. The test is reduced from that specific application and reproduces the issue with the loop-vectorizer pass. However, to me, it seemed that the issue was coming from the cost model.

jrtc27

Thanks

jrtc27 · 2024-05-28T19:28:44Z

(I suggest putting [X86] in the commit title though)

psteinfeld · 2024-05-29T12:23:11Z

@jplehr, here's a small test case the reproduces the original crash. I've tested the changes in this merge request, and they seem to fix it.

// clang -O2 -c -march=skylake-avx512 bug.c

typedef struct {
  const char *p;
  union {
    int i;
  } val;
} xcFooVal;

typedef struct {
  int n_vals;
  xcFooVal *vals;
} xcFooData;

typedef struct {
  xcFooData *bar;
  xcFooData *total_XX;
} YY_data;

extern xcFooData *XXbar, *XXbar1;

void func(const char *where) {
  YY_data data;
  data.bar = XXbar;
  data.total_XX = XXbar1;
  int i;
  for (i = 0; i < data.total_XX->n_vals; i++) {
    data.total_XX->vals[i].p = data.bar->vals[i].p;
  }
}

jplehr · 2024-05-29T12:24:52Z

Hi @psteinfeld thank you for that feedback.
Should this test case be added to the repository somewhere (and if so, should that be another PR)?

psteinfeld · 2024-05-29T12:33:36Z

Hi @psteinfeld thank you for that feedback. Should this test case be added to the repository somewhere (and if so, should that be another PR)?

I'm not sure. I thought you might need a test case in C for this current PR.

jplehr · 2024-05-29T12:41:07Z

I'm not sure. I thought you might need a test case in C for this current PR.

I'm gonna merge it as is, since it got accepted. If people feel that having a higher-level test is valuable it can be added.

jplehr requested review from arsenm and jrtc27 May 27, 2024 12:54

llvmbot added the backend:X86 label May 27, 2024

arsenm requested changes May 27, 2024

View reviewed changes

jplehr force-pushed the feat/fix-pr-92671 branch from 7b50987 to b4d3158 Compare May 28, 2024 15:00

llvmbot added the llvm:transforms label May 28, 2024

arsenm reviewed May 28, 2024

View reviewed changes

jplehr force-pushed the feat/fix-pr-92671 branch from b4d3158 to 8136b23 Compare May 28, 2024 18:05

llvmbot added the llvm:analysis Includes value tracking, cost tables and constant folding label May 28, 2024

jplehr force-pushed the feat/fix-pr-92671 branch from 8136b23 to afec059 Compare May 28, 2024 19:07

jrtc27 approved these changes May 28, 2024

View reviewed changes

jplehr requested a review from arsenm May 29, 2024 07:38

arsenm approved these changes May 29, 2024

View reviewed changes

jplehr merged commit 3082258 into llvm:main May 29, 2024
7 checks passed

steffenlarsen mentioned this pull request Jul 4, 2024

[CodeGen][X86] Use TargetLowering for TypeInfo of PointerTy (#93469) intel/llvm#14439

Merged

[CodeGen] Use TargetLowering for TypeInfo of PointerTy #93469

[CodeGen] Use TargetLowering for TypeInfo of PointerTy #93469

Uh oh!

Conversation

jplehr commented May 27, 2024

Uh oh!

llvmbot commented May 27, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm left a comment

Choose a reason for hiding this comment

Uh oh!

jplehr commented May 27, 2024

Uh oh!

jrtc27 commented May 27, 2024

Uh oh!

jplehr commented May 28, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

arsenm May 28, 2024

Choose a reason for hiding this comment

Uh oh!

jplehr May 28, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm May 28, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm May 28, 2024

Choose a reason for hiding this comment

Uh oh!

arsenm May 28, 2024

Choose a reason for hiding this comment

Uh oh!

jrtc27 commented May 28, 2024

Uh oh!

jrtc27 left a comment

Choose a reason for hiding this comment

Uh oh!

jrtc27 commented May 28, 2024

Uh oh!

psteinfeld commented May 29, 2024

Uh oh!

jplehr commented May 29, 2024

Uh oh!

psteinfeld commented May 29, 2024

Uh oh!

jplehr commented May 29, 2024

Uh oh!

Uh oh!

Uh oh!

llvmbot commented May 27, 2024 •

edited

Loading

jplehr commented May 28, 2024 •

edited

Loading