[AMDGPU][True16] set true16 mode as default on gfx110x

[broxigarchen]

Add RealTrue16Insts to FeatureISAVersion11_0_Common and set true16 mode as default on gfx110x.

[llvmbot]

@llvm/pr-subscribers-backend-amdgpu

Author: Brox Chen (broxigarchen)

Changes

Add RealTrue16Insts to FeatureISAVersion11_0_Common and set true16 mode as default on gfx110x

---

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

1 Files Affected:

• (modified) llvm/lib/Target/AMDGPU/AMDGPU.td (+2-1)

diff --git a/llvm/lib/Target/AMDGPU/AMDGPU.td b/llvm/lib/Target/AMDGPU/AMDGPU.td
index 764a7a9bf5e52..1120409de53a4 100644
--- a/llvm/lib/Target/AMDGPU/AMDGPU.td
+++ b/llvm/lib/Target/AMDGPU/AMDGPU.td
@@ -1800,7 +1800,8 @@ def FeatureISAVersion11_0_Common : FeatureSet<
     [FeatureMSAALoadDstSelBug,
      FeatureVALUTransUseHazard,
      FeatureMADIntraFwdBug,
-     FeaturePrivEnabledTrap2NopBug])>;
+     FeaturePrivEnabledTrap2NopBug,
+     FeatureRealTrue16Insts])>;
 
 def FeatureISAVersion11_0_0 : FeatureSet<
   !listconcat(FeatureISAVersion11_0_Common.Features,

[broxigarchen]

There are quite a number of changes being merged to enable the true16 mode on gfx11, and a set of tests are ran including lit test, cts test and performance test. We think it's the time now to try turning this mode on as default. Please let me know if there are still concerns to be address before merging this. Thanks!

[rampitec]

So what's about 16-bit inline asm constraints? Do we have it?

[broxigarchen]

So what's about 16-bit inline asm constraints? Do we have it?

Hi Stanislav, is it the "f16 inline interpreted as 16bit value" issue you are pointing to?

[rampitec]

So what's about 16-bit inline asm constraints? Do we have it?

Hi Stanislav, is it the "f16 inline interpreted as 16bit value" issue you are pointing to?

If instruction needs VGPR_16 the constraint "v" will supply VGPR_32. I suspect inline asm will not work in this situation. At the very least it needs to be verified and tested.

[broxigarchen]

So what's about 16-bit inline asm constraints? Do we have it?

Hi Stanislav, is it the "f16 inline interpreted as 16bit value" issue you are pointing to?

If instruction needs VGPR_16 the constraint "v" will supply VGPR_32. I suspect inline asm will not work in this situation. At the very least it needs to be verified and tested.

I see. Discussed offline with @Sisyph. I think the t16 mode asm inline is not yet supported. But I am not sure if it is blocking. The asm inline should still work with fake16 mode, and after this patch, the codegen can still be configured to use fake16 mode through the mattr setting, until when t16 inline is supported

[rampitec]

I see. Discussed offline with @Sisyph. I think the t16 mode asm inline is not yet supported. But I am not sure if it is blocking. The asm inline should still work with fake16 mode, and after this patch, the codegen can still be configured to use fake16 mode through the mattr setting, until when t16 inline is supported

I think it will be blocking, but it is not hard to add a new constraint.

[jayfoad]

What's the reason for restricting it to gfx110x? Are you just being cautious? Or are there known problems with gfx115x and gfx12?

[Sisyph]

What's the reason for restricting it to gfx110x? Are you just being cautious? Or are there known problems with gfx115x and gfx12?

There are known issues on both those subtargets.
For GFX115x, this is the only known issue. #138734
For GFX12, there are several things to implement, and it has not been tested yet.

[broxigarchen]

I see. Discussed offline with @Sisyph. I think the t16 mode asm inline is not yet supported. But I am not sure if it is blocking. The asm inline should still work with fake16 mode, and after this patch, the codegen can still be configured to use fake16 mode through the mattr setting, until when t16 inline is supported

I think it will be blocking, but it is not hard to add a new constraint.

Opened a PR here #140946

[arsenm]

Missing test changes? I'd expect at least something would change that shows the default

[broxigarchen]

Missing test changes? I'd expect at least something would change that shows the default

Hi Matt. Tests are handled seperately to address both true16 mode and non-true16 mode and the changes are merged already. We want this patch to be small so that if it hits some of the downstream branch it's easier to test and revert

[Sisyph]

Missing test changes? I'd expect at least something would change that shows the default

Hi Matt. Tests are handled and the changes are merged already. We want this patch to be small so that if it hits some of the downstream branch it's easier to test and revert

Do you WANT there to be a test change? We could have a small trivial test that shows the default. All the other tests have already been converted so they explicitly use +real-true16 or -real-true16 if there is a difference in output.

[arsenm]

Do you WANT there to be a test change? We could have a small trivial test that shows the default. All the other tests have already been converted so they explicitly use +real-true16 or -real-true16 if there is a difference in output.

I guess but it doesn't really matter