Page MenuHomePhabricator

[X86][AMX] Verify illegal types or instructions for x86_amx.
ClosedPublic

Authored by LuoYuanke on Wed, Apr 14, 5:20 AM.

Details

Summary

This patch is related to https://reviews.llvm.org/D100032 which define
some illegal types or operations for x86_amx. There are no arguments,
arrays, pointers, vectors or constants of x86_amx.

Diff Detail

Event Timeline

LuoYuanke created this revision.Wed, Apr 14, 5:20 AM
LuoYuanke requested review of this revision.Wed, Apr 14, 5:20 AM
Herald added a project: Restricted Project. · View Herald TranscriptWed, Apr 14, 5:20 AM
LuoYuanke updated this revision to Diff 337419.Wed, Apr 14, 5:26 AM

Remove attribute in test case.

lebedev.ri added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2414–2416

Not the bitcast, the load/store is the problem.

LuoYuanke updated this revision to Diff 337425.Wed, Apr 14, 5:44 AM

Address Roman's comments.

LuoYuanke added inline comments.Wed, Apr 14, 5:45 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
2414–2416

Since x86_amx pointer is invalid (llvm/test/Verifier/x86_amx8.ll), the load/store form x86_amx is also invalid. I change it as transform in the comments.

pengfei accepted this revision.Fri, Apr 16, 6:50 PM

LGTM, let's see if others have objections.

This revision is now accepted and ready to land.Fri, Apr 16, 6:50 PM
fhahn added inline comments.Sun, Apr 18, 6:01 AM
llvm/test/Verifier/x86_amx1.ll
4

can all those tests be in a single test file? I think the other verifier tests also group multiple checks in a single file

LuoYuanke added inline comments.Mon, Apr 19, 6:45 AM
llvm/test/Verifier/x86_amx1.ll
4

Probably not. As llvm-as stops on the first error.

; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s

define dso_local void @f() {
entry:
  %a.addr = alloca <2 x x86_amx>, align 4
  ret void
}

; CHECK: invalid vector element type


define void @f(x86_amx %A, x86_amx %B) {
entry:
  alloca x86_amx
; CHECK: invalid type for alloca
  ret void
}
fhahn added inline comments.Mon, Apr 19, 6:49 AM
llvm/test/Verifier/x86_amx1.ll
4

I'm not sure if there's a problem with the test matrix verifier tests or another difference, but I *think* the matrix verifier tests check multiple errors in a single file for example: https://github.com/llvm/llvm-project/blob/main/llvm/test/Verifier/matrix-intrinsics.ll

LuoYuanke added inline comments.Mon, Apr 19, 6:59 AM
llvm/test/Verifier/x86_amx1.ll
4

I tested llvm/test/Verifier/matrix-intrinsics.ll. Yes this case is able to be grouped in a single file, but it is not the same to x86_amx test case.

[llvm]$ cat test/Verifier/x86_amx.ll
; RUN: not llvm-as %s -o /dev/null 2>&1 | FileCheck %s

define dso_local void @f() {
entry:
  %a.addr = alloca <2 x x86_amx>, align 4
  ret void
}

; CHECK: invalid vector element type

define void @f(x86_amx %A, x86_amx %B) {
entry:
  alloca x86_amx
; CHECK: invalid type for alloca
  ret void
}
[llvm]$ llvm-as test/Verifier/x86_amx.ll
llvm-as: test/Verifier/x86_amx.ll:5:25: error: invalid vector element type
  %a.addr = alloca <2 x x86_amx>, align 4
                        ^
[llvm]$
LuoYuanke added inline comments.Mon, Apr 19, 7:01 AM
llvm/test/Verifier/x86_amx1.ll
4

It seems the token test cases are also not able to be grouped.
test/Verifier/token1.ll
test/Verifier/token2.ll
test/Verifier/token3.ll

craig.topper added inline comments.Mon, Apr 19, 10:29 AM
llvm/lib/IR/Verifier.cpp
2511

Should this be Function rather than Functions? Same for the message it was copied from

LuoYuanke updated this revision to Diff 338693.Mon, Apr 19, 8:05 PM

Address Craig's comments.

LuoYuanke added inline comments.Mon, Apr 19, 8:06 PM
llvm/lib/IR/Verifier.cpp
2511

Thanks! It's very careful of you. :)