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.

Sign up for GitHub

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

Jump to bottom

First stage of intrinsics support #111

Merged
merged 2 commits into from
Sep 27, 2024
Merged

First stage of intrinsics support #111

merged 2 commits into from
Sep 27, 2024

Conversation

thomasgoodfellow
Copy link
Collaborator

Implementation has substantial limitations:

  1. only tested against the demo xexample.subincacc instruction
  2. only tested via shallow object-code-analysis; needs running through ETISS to be confident registers correctly wrangled, etc
  3. only implemented 32-bit integer registers; needs more data types
  4. specification of intrinsics is purloined from S4E phase 1, using a separate YML file. Better approach would be embedding it in CoreDSL per the separate mail thread

Putting it forward for merging for feedback on changes so far

PhilippvK
PhilippvK reviewed Sep 10, 2024
View reviewed changes
seal5/pass_list.py Outdated Show resolved Hide resolved
PhilippvK
PhilippvK reviewed Sep 10, 2024
View reviewed changes
seal5/pass_list.py Outdated Show resolved Hide resolved
@PhilippvK
Copy link
Contributor

@thomasgoodfellow Thanks for the patch! Please look into the linter messages. Most of them can be fixed automatically with black. Would you prefer to make Python v3.10 the minimum Python version or will you revert the match statement into a if/elif/else?

@thomasgoodfellow
Basic support for generating intrinsic functions
64eb705 
Partially satisfies issue #66. Caveats abound:
1. only tested with the example subincacc instruction
2. generated code has yet to be tested in execution (e.g. ETISS)
3. intrinsics described through separate YML file; would be better
expressed in CoreDSL alongside the instruction

That the intrinsic support is partially implemented through the
riscv_instr_info is ungainly but made the dependency between the
instruction definition and the intrinsic trivial. The improved intrinsic
support in LLVM19 onwards may help to keep the implementation in the
riscv_intrinsics module.
@PhilippvK PhilippvK changed the base branch from main to develop September 12, 2024 08:36
@PhilippvK
Copy link
Contributor

PhilippvK commented Sep 12, 2024 •
edited
Loading

@thomasgoodfellow After merging this we can extend the tests by the intrinsics defined by the Core-V extensions:

Reference LLVM patches can be found here:

I can look into coming up with the YAML config files to define the intrinsics. First I would like to make sure https://github.com/tum-ei-eda/seal5/tree/corev-tests (#113) get's merged, as it contains baseline tests for Core-V functionalities.

PhilippvK
PhilippvK reviewed Sep 12, 2024
View reviewed changes
examples/cfg/example/intrinsics.yml
@@ -0,0 +1,13 @@
---
intrinsics:
intrinsics:
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@thomasgoodfellow What is the reason for the nested intrinsics.intrinsics here?

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That's regrettably ugly, emerging from my (mis?)use of dacite to have an optional list of intrinsic definitions. I'd certainly like to remove one layer of that onion but the formulations I tried didn't parse the YML I fed them

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would propose to move the intrinsics definitions under extensions: as the intrinstics are usually set specific. But this can be done in a followup-PR

Copy link
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Makes sense. BTW are you back earlier than scheduled, or not really back?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I will be officially back on Tuesday. But I got some time to work on my personal coding projects and while I am waiting for docker images to build, I can do some Seal5-related work ;)

@PhilippvK
Copy link
Contributor

When going to the Core-V intrinsics I saw that instructions with immediate operands might needs further attention to be supported: https://github.com/openhwgroup/corev-llvm-project/blob/b351908a2d564b2a4bb165d34d80063c4e3dd1b7/llvm/include/llvm/IR/IntrinsicsRISCV.td#L1743

@thomasgoodfellow Do you think that the information about ImmArg<ArgIndex<2> would be infered automatically by Seal5 or should this be explicitly part of the YAML settings?

@PhilippvK
Copy link
Contributor

PhilippvK commented Sep 12, 2024 •
edited
Loading

@thomasgoodfellow Here is the initial YAML for the Core-V intrinsics I came up with. Fortunately I could write it down in a compact fashion using YAML Anchors (https://www.linode.com/docs/guides/yaml-anchors-aliases-overrides-extensions/#yaml-overrides, Any YAML parser including PyYAML should extend it automatically)

---  # examples/cfg/xcorev/intrinsics.yml
intrinsics:
  intrinsics:
  # XCoreVAlu
  - &ScalarCoreVAluGprIntrinsic
    args: 
    - arg_name: rs1
      arg_type: i32
    instr_name: XCoreVAlu.SEAL5_CV_EXTHS
    intrinsic_name: riscv_cv_alu_exths
    ret_type: i32
  - <<: *ScalarCoreVAluGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_EXTHZ
    intrinsic_name: int_riscv_cv_alu_exthz
  - <<: *ScalarCoreVAluGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_EXTBS
    intrinsic_name: int_riscv_cv_alu_extbs
  - <<: *ScalarCoreVAluGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_EXTBZ
    intrinsic_name: int_riscv_cv_alu_extbz
  - &ScalarCoreVAluGprGprIntrinsic
    args: 
    - arg_name: rs1
      arg_type: i32
    - arg_name: rs2
      arg_type: i32
    instr_name: XCoreVAlu.SEAL5_CV_SLET
    intrinsic_name: riscv_cv_alu_slet
    ret_type: i32
  - <<: *ScalarCoreVAluGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_SLETU
    intrinsic_name: riscv_cv_alu_sletu
  - <<: *ScalarCoreVAluGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_MIN
    intrinsic_name: riscv_cv_alu_min
  - <<: *ScalarCoreVAluGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_MINU
    intrinsic_name: riscv_cv_alu_minu
  - <<: *ScalarCoreVAluGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_MAX
    intrinsic_name: riscv_cv_alu_max
  - <<: *ScalarCoreVAluGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_MAXU
    intrinsic_name: riscv_cv_alu_maxu
  - <<: *ScalarCoreVAluGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_clip
    intrinsic_name: riscv_cv_alu_clip
  - <<: *ScalarCoreVAluGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_CLIPU
    intrinsic_name: riscv_cv_alu_clipu
  - &ScalarCoreVAluGprGprGprIntrinsic
    args: 
    - arg_name: rs1
      arg_type: i32
    - arg_name: rs2
      arg_type: i32
    - arg_name: rs3  # TODO: Shouldn't this be Luimm5 and immediate: true?
      arg_type: i32
    instr_name: XCoreVAlu.SEAL5_CV_ADDN
    intrinsic_name: riscv_cv_alu_addN
    ret_type: i32
  - <<: *ScalarCoreVAluGprGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_ADDUN
    intrinsic_name: riscv_cv_alu_adduN
  - <<: *ScalarCoreVAluGprGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_ADDRN
    intrinsic_name: riscv_cv_alu_addRN
  - <<: *ScalarCoreVAluGprGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_ADDURN
    intrinsic_name: riscv_cv_alu_adduRN
  - <<: *ScalarCoreVAluGprGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_SUBN
    intrinsic_name: riscv_cv_alu_subuN
  - <<: *ScalarCoreVAluGprGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_SUBUN
    intrinsic_name: riscv_cv_alu_subuN
  - <<: *ScalarCoreVAluGprGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_SUBRN
    intrinsic_name: riscv_cv_alu_subRN
  - <<: *ScalarCoreVAluGprGprGprIntrinsic
    instr_name: XCoreVAlu.SEAL5_CV_SUBURN
    intrinsic_name: riscv_cv_alu_subuRN
  # XCoreVMac
  - &ScalarCoreVMacGprGprGprIntrinsic
    args: 
    - arg_name: rd
      arg_type: i32
    - arg_name: rs1
      arg_type: i32
    - arg_name: rs2
      arg_type: i32
    instr_name: XCoreVMac.SEAL5_CV_MAC
    intrinsic_name: riscv_cv_mac_mac
    ret_type: i32
  - <<: *ScalarCoreVMacGprGprGprIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MSU
    intrinsic_name: mac_msu
  - &ScalarCoreVMacGprGPRImmIntrinsic
    args: 
    - arg_name: rs1
      arg_type: i32
    - arg_name: rs2
      arg_type: i32
    - arg_name: Is3
      arg_type: i32
      immediate: true
    instr_name: XCoreVMac.SEAL5_CV_MULUN
    intrinsic_name: riscv_cv_mac_muluN
    ret_type: i32
  - <<: *ScalarCoreVMacGprGPRImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MULHHUN
    intrinsic_name: riscv_cv_mac_mulhhuN
  - <<: *ScalarCoreVMacGprGPRImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MULSN
    intrinsic_name: riscv_cv_mac_mulsN
  - <<: *ScalarCoreVMacGprGPRImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MULHHSN
    intrinsic_name: riscv_cv_mac_mulhhsN
  - <<: *ScalarCoreVMacGprGPRImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MULURN
    intrinsic_name: riscv_cv_mac_muluRN
  - <<: *ScalarCoreVMacGprGPRImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MULHHURN
    intrinsic_name: riscv_cv_mac_mulhhuRN
  - <<: *ScalarCoreVMacGprGPRImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MULSRN
    intrinsic_name: riscv_cv_mac_mulsRN
  - <<: *ScalarCoreVMacGprGPRImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MULHHSRN
    intrinsic_name: riscv_cv_mac_mulhhsRN
  - &ScalarCoreVMacGprGprGprImmIntrinsic
    args: 
    - arg_name: rd
      arg_type: i32
    - arg_name: rs1
      arg_type: i32
    - arg_name: rs2
      arg_type: i32
    - arg_name: Is3
      arg_type: i32
      immediate: true
    instr_name: XCoreVMac.SEAL5_CV_MACUN
    intrinsic_name: riscv_cv_mac_macuN
    ret_type: i32
  - <<: *ScalarCoreVMacGprGprGprImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MACHHUN
    intrinsic_name: riscv_cv_mac_machhuN
  - <<: *ScalarCoreVMacGprGprGprImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MULSN
    intrinsic_name: riscv_cv_mac_macsN
  - <<: *ScalarCoreVMacGprGprGprImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MACHHSN
    intrinsic_name: riscv_cv_mac_machhsN
  - <<: *ScalarCoreVMacGprGprGprImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MACURN
    intrinsic_name: riscv_cv_mac_macuRN
  - <<: *ScalarCoreVMacGprGprGprImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MACHHURN
    intrinsic_name: riscv_cv_mac_machhuRN
  - <<: *ScalarCoreVMacGprGprGprImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MACSRN
    intrinsic_name: riscv_cv_mac_macsRN
  - <<: *ScalarCoreVMacGprGprGprImmIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_MACHHSRN
    intrinsic_name: riscv_cv_mac_machhsRN
  # XCoreVSimd
  - &ScalarCoreVSimdGprGprIntrinsic
    args: 
    - arg_name: rs1
      arg_type: i32
    - arg_name: rs2
      arg_type: i32
    instr_name: XCoreVSimd.SEAL5_CV_ADD_B
    intrinsic_name: riscv_cv_simd_add_b
    ret_type: i32
  - <<: *ScalarCoreVSimdGprGprIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_ADD_H
    intrinsic_name: riscv_cv_simd_add_h
  - <<: *ScalarCoreVSimdGprGprIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_ADD_SC_B
    intrinsic_name: riscv_cv_simd_add_sc_b
  - <<: *ScalarCoreVSimdGprGprIntrinsic
    instr_name: XCoreVMac.SEAL5_CV_ADD_SC_H
    intrinsic_name: riscv_cv_simd_add_sc_h
  # TODO:
  # CoreVSimdBinary: sub, avg, avgu, min, minu, max, maxum srl, sra, sll, or, xor, and, dotup, dotusp, dotsp, sdotup, sdotusp, sdotsp
  #                  cmpeq, cmpne, cmpgt, cmpge, cmplt, cmple, cmpgtu, cmpgeu, cmpltu, cmpleu
  # ScalarCoreVSimdGprIntrinsicHB: abs
  # ScalarCoreVSimdGprImmIntrinsicHB: extract, extractu, 
  # ?: insert_b, insert_h, 
  # ScalarCoreVSimdGprGprIntrinsicHB: shuffle 
  # ScalarCoreVSimdGprImmIntrinsic: shuffle_sci_h, shuffle_sci_b
  # ScalarCoreVSimdGprGprGprIntrinsicHB: shuffle2 
  # ScalarCoreVSimdGprGprIntrinsic: packhi_h, packlo_h
  # ScalarCoreVSimdGprGprGprIntrinsic: packhi_b, packlo_b
  # ScalarCoreVSimdGprGprGprImmIntrinsic: cplxmul_r, cplxmul_i
  # ScalarCoreVSimdGprIntrinsic: cplxconj, 
  # ScalarCoreVSimdGprGprImmIntrinsic: subrotmj
  #
  # TODO: XCoreVBitmanip (not fully implemented in CDSL)
  # int_riscv_cv_bitmanip_ff1
  # int_riscv_cv_bitmanip_fl1 
  # int_riscv_cv_bitmanip_insert
  # int_riscv_cv_bitmanip_clb 
  # int_riscv_cv_bitmanip_cnt 
  # int_riscv_cv_bitmanip_bitrev
  # int_riscv_cv_bitmanip_extract
  # int_riscv_cv_bitmanip_extractu
  # int_riscv_cv_bitmanip_bclr
  # int_riscv_cv_bitmanip_bset

@thomasgoodfellow
Intrinsic for example subincacc instruction
c93d63b 
Definition and test case for subincacc. Partially addresses #67 and #68
@PhilippvK
Copy link
Contributor

Let‘s get this merged now. I will fixup the linter issues on develop

@PhilippvK PhilippvK merged commit 3bd8a58 into develop Sep 27, 2024
4 of 6 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@PhilippvK PhilippvK PhilippvK left review comments

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@thomasgoodfellow @PhilippvK