Bug 50317

Amnesia game also hit that error with llvm

OpenGL renderer string: Gallium 0.4 on AMD RV770
OpenGL version string: 2.1 Mesa 8.1-devel (git-529476b)

R6000_LLVM=0 workaround the problem.

Hit by this bug when trying flightgear on latest libdrm/mesa/xorg-server/xf86-video-ati. All compiled today from git/svn/whatever:

LLVM ERROR: Cannot select: target intrinsic %llvm.AMDGPU.sin

Just hit this bug with HoN.
Apparently there is a pattern for AMDIL.sin, but not for AMDGPU.sin:

class SIN_Common <bits<32> inst> : R600_1OP <
  inst, "SIN",
  [(set R600_Reg32:$dst, (int_AMDIL_sin R600_Reg32:$src))]>{
  let Trig = 1;
}

(In reply to comment #3)
> Just hit this bug with HoN.
> Apparently there is a pattern for AMDIL.sin, but not for AMDGPU.sin:
> 
> class SIN_Common <bits<32> inst> : R600_1OP <
>   inst, "SIN",
>   [(set R600_Reg32:$dst, (int_AMDIL_sin R600_Reg32:$src))]>{
>   let Trig = 1;
> }

That pattern in the SIN_Common class is stale and should be removed.

For evergreen and newer, there is a pattern for int_AMGPU_sin:

def : TRIG_eg <SIN_eg, int_AMDGPU_sin>;

R700 chips normalize trigonometric inputs the same was as Evergreen+, so the correct fix would be to create a SIN_r700 instruction and use the TRIG_eg class to create a pattern for it.  This will also require adding a predicate to identify r700 chips, because r600 normalizes the inputs differently.

Created attachment 63183 [details] [review]
SIN/COS_r700 as suggested

(In reply to comment #4)
> 
> R700 chips normalize trigonometric inputs the same was as Evergreen+, so the
> correct fix would be to create a SIN_r700 instruction and use the TRIG_eg class
> to create a pattern for it.

Thanks, the attached patch seems to fix the problem for me.

>  This will also require adding a predicate to
> identify r700 chips, because r600 normalizes the inputs differently.

Oldest chip listed is OCL_DEVICE_RV710, so it would appear that R600_LLVM
only supports chips >= RV710 and doesn't support R600 at all. Am I missing something?
Anyway the patch checks for HD4XXX and >= RV710.

(In reply to comment #5)
> Created attachment 63183 [details] [review] [review]
> SIN/COS_r700 as suggested

This patch makes HoN stop crashing, but it'll actually fall back to using TGSI (due to indirect addressing) so I don't know if the rendering would be correct.
Piglit tests about sin/cos pass though.

(In reply to comment #6)
> (In reply to comment #4)
> > 
> > R700 chips normalize trigonometric inputs the same was as Evergreen+, so the
> > correct fix would be to create a SIN_r700 instruction and use the TRIG_eg class
> > to create a pattern for it.
> 
> Thanks, the attached patch seems to fix the problem for me.
> 
> >  This will also require adding a predicate to
> > identify r700 chips, because r600 normalizes the inputs differently.
> 
> Oldest chip listed is OCL_DEVICE_RV710, so it would appear that R600_LLVM
> only supports chips >= RV710 and doesn't support R600 at all. Am I missing
> something?
> Anyway the patch checks for HD4XXX and >= RV710.
> 
Currently R600 chips are treated as RV710 chips, which is another bug.  To fix this, we need to modify the r600_llvm_gpu_string() function in r600_llvm.c to return "r600" for R600 chips.  Then we need to add a processor type for it in Processors.td, and finally we need to handle the "r600" processor in AMDIL7XXDevice.cpp (No need to create a AMDIL6XXDevice class). For R600 mDeviceFlag should be 0.

Comment on attachment 63183 [details] [review]
SIN/COS_r700 as suggested

> 
>@@ -809,13 +812,21 @@ def RECIP_UINT_eg : RECIP_UINT_Common<0x94>;
> /* Evergreen / Cayman Instructions */
> /* ------------------------------- */
> 
>-let Predicates = [isEGorCayman] in {
>-  
> class TRIG_eg <InstR600 trig, Intrinsic intr> : Pat<
>   (intr R600_Reg32:$src),
>   (trig (MUL (MOV_IMM_I32 (i32 ALU_LITERAL_X), CONST.TWO_PI_INV), R600_Reg32:$src))
> >;
> 
>+let Predicates = [isR700] in {
>+  /* R700 normalizes inputs to SIN/COS the same as EG */
>+  def SIN_r700 : SIN_Common<0x6E>;
>+  def COS_r700 : COS_Common<0x6F>;
>+
>+  def : TRIG_eg <SIN_r700, int_AMDGPU_sin>;
>+  def : TRIG_eg <COS_r700, int_AMDGPU_cos>;
>+}

Can you move these R700 instruction definitions above the Evergreen Only instructions, so the chip definitions are in chronological order.  There should also be an 'R700 Only Instructions' header comment similar to the 'Evergreen Only' header.

Could you also delete the TRIG_HELPER_r700 class as well as the header comment above it now that it is no longer needed.

Otherwise looks good. If possible, use git-format-patch when you repost.  It will make it easier for me to commit and push.

Created attachment 63186 [details] [review]
v2 R700 sin/cos

moved instructions above EG. Also removed obsolete int_AMDGPU_cos pattern similar to the sin one.

Created attachment 63187 [details] [review]
treat r600 separately from rv710

Note: Processors.td has wrong EOLs, you'll probably need to dos2unix before applying this patch.

Created attachment 63188 [details] [review]
dos2unix Processors.td

git doesn't like it otherwise.

To apply:
git am --ignore-whitespace 0002-radeon-llvm-fix-CR-LF.patch
git am 0003-radeon-llvm-treat-r600-separately-from-rv710.patch

(In reply to comment #8)
> 
> Can you move these R700 instruction definitions above the Evergreen Only
> instructions, so the chip definitions are in chronological order.  There should
> also be an 'R700 Only Instructions' header comment similar to the 'Evergreen
> Only' header.
> 
> Could you also delete the TRIG_HELPER_r700 class as well as the header comment
> above it now that it is no longer needed.
> 
> Otherwise looks good. If possible, use git-format-patch when you repost.  It
> will make it easier for me to commit and push.

Done.

(In reply to comment #7)

> Currently R600 chips are treated as RV710 chips, which is another bug.  To fix
> this, we need to modify the r600_llvm_gpu_string() function in r600_llvm.c to
> return "r600" for R600 chips.  Then we need to add a processor type for it in
> Processors.td, and finally we need to handle the "r600" processor in
> AMDIL7XXDevice.cpp (No need to create a AMDIL6XXDevice class). For R600
> mDeviceFlag should be 0.

Attached a patch for this, Processors.td has wrong EOLs though so you may need to apply 0002 to clean up the CR/LF, and then 0003.

BTW I noticed that RV670 is treated as R700, but according to RadeonFeature on the wiki thats a R600 class GPU. Which one is right?

(In reply to comment #13)
> (In reply to comment #7)
> 
> > Currently R600 chips are treated as RV710 chips, which is another bug.  To fix
> > this, we need to modify the r600_llvm_gpu_string() function in r600_llvm.c to
> > return "r600" for R600 chips.  Then we need to add a processor type for it in
> > Processors.td, and finally we need to handle the "r600" processor in
> > AMDIL7XXDevice.cpp (No need to create a AMDIL6XXDevice class). For R600
> > mDeviceFlag should be 0.
> 
> Attached a patch for this, Processors.td has wrong EOLs though so you may need
> to apply 0002 to clean up the CR/LF, and then 0003.
> 
> BTW I noticed that RV670 is treated as R700, but according to RadeonFeature on
> the wiki thats a R600 class GPU. Which one is right?

Also I don't have a R600 to test, and this probably doesn't fix sin/cos on the R600, so I'll leave that for someone else to fix.

(In reply to comment #13)
> BTW I noticed that RV670 is treated as R700, but according to RadeonFeature on
> the wiki thats a R600 class GPU. Which one is right?

We probably need to add several strings for r6xx asics as there are some slight variants between the various r6xx asics.  Something like:

r600 - R600
rv6xx - RV610, RV630, RV620, RV635, RS780, RS880
rv670 - RV670

RV670 supports doubles and a couple of other features that are not supported by other r6xx asics.

This bug is fixed in git by commit 7c005d5687eeca5b3115f69a681d21e61741c5dc

I pushed the sin/cos and dos2unix patches.  I think the third patch that teaches the backend about r600 should be modified to include the changes suggest by Alex.  If you want to respin the patch and send it to the mailing list I'll review/commit it.

Thanks for the patches!

reopened because r600 is still missing sin/cos functions.

r600 + llvm is never going to be completed. Closing as won't fix.