Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

BLD: a few XL compiler observations #2333

Closed
tylerjereddy opened this issue Dec 3, 2019 · 6 comments
Closed

BLD: a few XL compiler observations #2333

tylerjereddy opened this issue Dec 3, 2019 · 6 comments

Comments

@tylerjereddy
Copy link
Contributor

I know we're not "supposed" to patch OpenBLAS code to work with XLC on POWER9 because those are mostly issues with the compiler/toolchain upstream rather than with OpenBLAS, as previously noted in a few places (by i.e., @edelsohn ).

Nonetheless, I tried to apply some of the hints on registers from #1699 and #1078 (comment).

I'm doing the build in a spack-based context, and after applying the various vs32 -> v0 and =wa -> =v substitutions there seems to be some assembly-related choking, which is perhaps not surprising, but just to report an example of it using latest OpenBLAS develop branch:

  >> 1382    srot_k$1.s:102: Error: syntax error; found `(', expected `,'
  >> 1383    srot_k$1.s:102: Error: junk at end of line: `(6),50,37'
  >> 1384    srot_k$1.s:103: Error: syntax error; found `(', expected `,'
  >> 1385    srot_k$1.s:103: Error: junk at end of line: `(4),51,37'
  >> 1386    srot_k$1.s:110: Error: syntax error; found `(', expected `,'
  >> 1387    srot_k$1.s:110: Error: junk at end of line: `(6)'
  >> 1388    srot_k$1.s:111: Error: syntax error; found `(', expected `,'
  >> 1389    srot_k$1.s:111: Error: junk at end of line: `(4)'
  >> 1390    srot_k$1.s:143: Error: syntax error; found `(', expected `,'
  >> 1391    srot_k$1.s:143: Error: junk at end of line: `(6),50,37'
  >> 1392    srot_k$1.s:144: Error: syntax error; found `(', expected `,'
  >> 1393    srot_k$1.s:144: Error: junk at end of line: `(4),51,37'
  >> 1394    srot_k$1.s:149: Error: syntax error; found `(', expected `,'
  >> 1395    srot_k$1.s:149: Error: junk at end of line: `(6)'
  >> 1396    srot_k$1.s:150: Error: syntax error; found `(', expected `,'
  >> 1397    srot_k$1.s:150: Error: junk at end of line: `(4)'

The problem lines (i.e., 102, 103, 110, 111 shown below & complained about above) from srot_k$1.s always seem to have parentheses:

102     xvmulsp     0(6), 50, 37   
103     xvmulsp     0(4), 51, 37 
104     lxvd2x      50, 7, 10
105     lxvd2x      51, 8, 10   
106     xvaddsp     40, 40, 59  
107     xvaddsp     41, 41, 58  
108     addi        9, 9, -64   
109     addi        10, 10, -64  
110     xvaddsp     42, 42, 0(6)
111     xvaddsp     43, 43, 0(4)  

That's way over my head, but anyway just passing along in case it is helpful or if there are things you'd like me to try there. I did mess around with COMMON_OPT=-mcpu=power9 -mtune=power9 -Ofast and an empty COMMON_OPT to get the same result using spack's build system.

cc @AndrewGaspar @gshipman @junghans

@edelsohn
Copy link

edelsohn commented Dec 3, 2019

I'm not certain exactly what patches you are trying. "v" and "wa" register constraints are not interchangeable. I don't know what XLC supports.

Originally there were the Altivec/VMX registers ("v"). Then Power added VSX registers, which combine the VMX registers with the FP registers. The mnemonics for both original register files started with 0 and only one could win for retaining the meaning of 0: the VRs or the FPRs.

"v" emits the appropriate register numbers for Altivec instructions and "wa" emits the appropriate register numbers for VSX instructions. Altivec and VSX instructions can be intermingled, but one must use the appropriate subset of 32 Altivec-only registers and one must adjust the register numbers by 32, depending on the instruction.

The 0(4) and 0(6) don't make any sense in that context. Those normally would be displacement addresses, or indirect addresses in this case because the "0" means displacement 0. "0(4)" would represent 0 displacement relative to register 4. But the instructions are not loads or stores, so an address operand is completely wrong. Something must be using the completely wrong output template in inlined assembly to emit an address where a plain register should appear.

@brada4
Copy link
Contributor

brada4 commented Dec 3, 2019

Could you supply full patch that breaks the build for you?
Power 8 and 9 _rot are built from generic C codes that do not exceed 200 lines.

@tylerjereddy
Copy link
Contributor Author

The patch file is below (from git diff > openblas_xl.txt), or you can check the feature branch treddy_openblas_xl_experiment on my fork vs. develop branch.

openblas_xl.txt

Without the patch I got assembly complaints that have been described in the previous issues linked above. I can get you more detailed log files if those are helpful.

@martin-frbg
Copy link
Collaborator

martin-frbg commented Dec 3, 2019

The confused assembly comes from the srot_microk_power8.c included by srot.c, which has

       "xvmulsp         %x11, 50, 37    \n\t"
       "xvmulsp         %x12, 51, 37    \n\t"

       "lxvd2x          50, %16, %4     \n\t"
       "lxvd2x          51, %17, %4     \n\t"

etc. starting from line 107, suggesting there is something wrong with the contents of %x11 and %x12. (srot_microk_power8.c was basically rewritten by Alan Modra in #1078 , I only applied the patch he submitted)

@edelsohn
Copy link

edelsohn commented Dec 3, 2019

First, please don't change the code to allow it to support XLC. One should not limit the registers to Altivec/VMX because of XLC bugs.

Second, I have no idea if XLC implements the '%x' output format correctly.

XLC is completely the wrong priority. Please stop any activities to accommodate XLC.

@tylerjereddy
Copy link
Contributor Author

Ok, I was just confirming that this roadblock seems reasonable/still exists. Sounds like it is still a blocker. I think we have a way to communicate with the XL compiler team from LANL so I'll see if the group wants to ask the IBM folks about that later this week. Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

4 participants