This is an archive of the discontinued LLVM Phabricator instance.

[IR][X86] Remove X86AMX type in LLVM IR instead of target extension
Needs RevisionPublic

Authored by zixuan-wu on Jan 17 2023, 12:13 AM.

Details

Summary

As the patch https://reviews.llvm.org/D135202 introduces the target extension type, we can represent higher level or complex type as single value types which are valid in target specific registers from CodeGen’s perspective in LLVM IR. So we can remove some target specific type already in LLVM IR such as x86amx and represent them by target extension type consistently.

This patch removes X86AMX type in LLVM IR and replace it with target extension called target("X86.AMX"). It also does not change anything in backend and only add two bitcast intrinsics to express bitcast between <256 x i32> and target("X86.AMX") because bitcast inst is not allowed for target extension in LLVM IR. It needs to try to remove bitcast intrinsics in next follow-up patch, which I think there are more important work in clang about how to support target extension type instead of using V256Ii type flags in builtin parameters.

Diff Detail

Event Timeline

zixuan-wu created this revision.Jan 17 2023, 12:13 AM
zixuan-wu requested review of this revision.Jan 17 2023, 12:13 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 17 2023, 12:13 AM

@zixuan-wu, changing x86_amx would break our internal code. May I know the motivation to change the type?

@zixuan-wu, changing x86_amx would break our internal code. May I know the motivation to change the type?

I just want to point out that generally "causes [too much] churn downstream"
is not relevant concern upstream, as downstreams are considered to be on their own.

  • Can "x86.AMX" be a named constant somewhere? I found a lot of copies.
  • Release note?

@zixuan-wu, changing x86_amx would break our internal code. May I know the motivation to change the type?

I just want to point out that generally "causes [too much] churn downstream"
is not relevant concern upstream, as downstreams are considered to be on their own.

But we previously design the amx type and we follow the design to develop and maintain both the upsteam code and downstream code. I think we should respect the orginal author's effort for developing and maintaining the code.

The summary only says what you are doing. I am missing something about why.

nikic added a comment.Jan 17 2023, 6:45 AM

From a very cursory look, this looks pretty nice. It looks like we pretty much get rid of AMX-specific knowledge in the middle-end entirely, which is the point of target extension types.

Regarding bitcasts, it's worth mentioning that there was some recent discussion about allowing bitcasts on target types as one of the supported properties -- however, I think we wouldn't actually want to do that for AMX, because of the special properties it has. In particular, if we allow bitcasts, we should also allow bitcast optimizations, and those are generally illegal for AMX (as seen with the various checks this patch removes).

Something this is missing is auto-upgrade support in the bitcode reader.

Nitpick: I wonder whether the type should be called target("x86.amx") -- it seems like lowercase names are more idiomatic in LLVM? No strong feelings either way though.

clang/lib/CodeGen/CGBuiltin.cpp
5338

Probably worthwhile to add an overload like Type::isTargetExtTy(StringRef)?

5373

Something I don't really get is why we need the separate bitconvert intrinsic, and the existing cast intrinsic does not work. Do they have different semantics?

nikic added a comment.Jan 17 2023, 7:06 AM

I think it would also be fine to retain the Type::isX86_AMXTy(), at least for now. That would reduce this patch to mostly the representation change, without the change to check for the type in a different way.

llvm/lib/IR/Core.cpp
655

These methods can be kept and just create the target type instead.

From some of the verifier checks and tests, it looks like target("x86.amx") would also require some new type properties, to express its unsuitability for alloca-and-friends, as well as non-intrinsic arguments.

The spelling change needs to be release noted, and I would like there to be some mention of the the type in documentation going forward, but it seems that X86 doesn't have a target-specific documentation page yet.

clang/lib/CodeGen/CGBuiltin.cpp
5338

+1 to a method for this pattern.

llvm/include/llvm-c/Core.h
168

Removing this enum value changes the ABI. I don't think we've removed a type before, but with analogy to the opcode enum above, we should probably explicitly enumerate the type kinds and leave a hole for where LLVMX86_AMXTypeKind used to be.

1555

Retaining the existing LLVM-C methods is possible without much maintenance overhead, so we should do so.

llvm/lib/IR/Type.cpp
843

If I'm following the verifier rules for x86_amx correctly, these are not in fact true for a target("x86.amx") type.

llvm/test/Verifier/x86_amx1.ll
2–3

It should be possible to retain these verifier tests even if x86_amx is moved to a target extension type, although the messages may need to change.

Could you please put an X86.rst file into llvm/docs? There is a Sphinx template with instructions.

We need consider how to be compatible with the existing software if we want the change the IR type. There are some existing software that is based on the existing type. For example the AMX dialect of MLIR and TLX code are based on the x86_amx, it would break them if we change the type. Some existing bitcode or text format IR already have already based on x86_amx. They will also be broken if we change the IR type. And I am also concerned about the following patch that remove amx cast intrinsics. We've done many optimization based on the intrinsics. Removing the intrinsics would break the optimization and all the code for the existing software.

Given the current patch is intrusive to current AMX solution, I would suggest author to describe the background and roadmap of your work and prototype the target extension type and its usage without changing x86_amx. If the prototype is good, we can discuss how to switch AMX to the new infrastructure without breaking existing software.

LuoYuanke requested changes to this revision.Jan 17 2023, 6:12 PM
This revision now requires changes to proceed.Jan 17 2023, 6:12 PM
zixuan-wu added a comment.EditedJan 17 2023, 10:04 PM

@zixuan-wu, changing x86_amx would break our internal code. May I know the motivation to change the type?

The background is at https://reviews.llvm.org/D135202. No more motivation, just to purify LLVM IR and demonstrate and validate target extension type. I think putting target-specific type into LLVM IR was thoughtless at that moment. Considering there was no better solution at that time such as target extension, it's a workable workaround. But it should not keep going anymore if there is better way.

LuoYuanke added a comment.EditedJan 17 2023, 10:18 PM

@zixuan-wu, changing x86_amx would break our internal code. May I know the motivation to change the type?

The background is at https://reviews.llvm.org/D135202. No more motivation, just to purify LLVM IR and demonstrate target extension. I think putting target-specific type into LLVM IR was thoughtless at that moment. Considering there was no better solution at that time such as target extension, it's a workable workaround. But it should not keep going anymore if there is better way.

I think target extension type is nice, if it is introduced 2 years ago I would vote for it. However my concern is the compatibility issue as I explained. We need to be compatible to the IR that built by previous compiler, and be compatible to the 3rd party software that based on the x86_amx type. I can't predict more risks for now if we replace an LLVM IR type, but I believe there is big risk hidden.

zixuan-wu edited the summary of this revision. (Show Details)Jan 17 2023, 10:27 PM
zixuan-wu added a reviewer: lattner.

@zixuan-wu, changing x86_amx would break our internal code. May I know the motivation to change the type?

The background is at https://reviews.llvm.org/D135202. No more motivation, just to purify LLVM IR and demonstrate and validate target extension type.

I think TLX may be a better case to demonstrate target extension type as TLX is pretty new.

zixuan-wu added a comment.EditedJan 17 2023, 10:50 PM

With considering https://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility I think we need make consensus to choose one option from following 2 options.

  1. Remove X86amx type in IR totally. (what I am doing now)
  2. Without removing X86amx type in IR, just upgrade the x86amx type to target extension and also upgrade bitcast llvm instruction to intrinsic(required). It also includes changing the testcase to target extension type.

BTW, need we make consensus about whether to do this purify patch? Everybody can make comments and preference about questions above.

With considering https://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility I think we need make consensus to choose one option from following 2 options.

  1. Remove X86amx type in IR totally. (what I am doing now)
  2. Without removing X86amx type in IR, just upgrade the x86amx type to target extension and also upgrade bitcast llvm instruction to intrinsic(required). It also includes changing the testcase to target extension type.

BTW, need we make consensus about whether to do this purify patch anymore?

For option 2 is it possible to make x86_amx and target extension type co-exist and have a knob to control it? The new feature can base on the target extension type, and if the target extension type is validated on more and more applications we need figure out a way to deprecate the old type someday.

I don't think we need to puirfy the patch anymore. I prefer to have 2 type co-exist for some time and deprecate 1 in a proper and safe way.

nikic added a comment.Jan 19 2023, 5:56 AM

With considering https://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility I think we need make consensus to choose one option from following 2 options.

  1. Remove X86amx type in IR totally. (what I am doing now)
  2. Without removing X86amx type in IR, just upgrade the x86amx type to target extension and also upgrade bitcast llvm instruction to intrinsic(required). It also includes changing the testcase to target extension type.

I believe the right option is:

  1. Remove x86_amx type from the (in-memory) IR representation, but support an auto-upgrade for bitcode only.

We do need bitcode auto-upgrade support as a matter of policy, and we shouldn't support both type representation at the same time, that would defeat the point of the change.

I think target extension type is nice, if it is introduced 2 years ago I would vote for it. However my concern is the compatibility issue as I explained. We need to be compatible to the IR that built by previous compiler, and be compatible to the 3rd party software that based on the x86_amx type. I can't predict more risks for now if we replace an LLVM IR type, but I believe there is big risk hidden.

Due to bitcode auto-upgrade, compatibility with old IR is retained. As long as we avoid some of the API changes here, the impact on downstream code should be fairly minimal. (Though as already pointed out, downstream impact generally doesn't figure into LLVM design decisions anyway.)

Matt added a subscriber: Matt.Jan 25 2023, 9:06 AM
skan added a subscriber: skan.Feb 5 2023, 10:38 PM

With considering https://llvm.org/docs/DeveloperPolicy.html#ir-backwards-compatibility I think we need make consensus to choose one option from following 2 options.

  1. Remove X86amx type in IR totally. (what I am doing now)
  2. Without removing X86amx type in IR, just upgrade the x86amx type to target extension and also upgrade bitcast llvm instruction to intrinsic(required). It also includes changing the testcase to target extension type.

I believe the right option is:

  1. Remove x86_amx type from the (in-memory) IR representation, but support an auto-upgrade for bitcode only.

We do need bitcode auto-upgrade support as a matter of policy, and we shouldn't support both type representation at the same time, that would defeat the point of the change.

I think target extension type is nice, if it is introduced 2 years ago I would vote for it. However my concern is the compatibility issue as I explained. We need to be compatible to the IR that built by previous compiler, and be compatible to the 3rd party software that based on the x86_amx type. I can't predict more risks for now if we replace an LLVM IR type, but I believe there is big risk hidden.

Due to bitcode auto-upgrade, compatibility with old IR is retained. As long as we avoid some of the API changes here, the impact on downstream code should be fairly minimal. (Though as already pointed out, downstream impact generally doesn't figure into LLVM design decisions anyway.)

Deal. And because I am on busy for a long time and it is also better to let intel guy handle x86-related feature, I am happy with the patch being commandeered.

Deal. And because I am on busy for a long time and it is also better to let intel guy handle x86-related feature, I am happy with the patch being commandeered.

@nikic's proposal looks a promising solution, we can investigate more about it.

barannikov88 resigned from this revision.May 11 2023, 6:27 AM