Skip to content

[clang][RISCV] Extend intrinsic size check variable from 16 -> 32 bits. NFC #111481

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

Merged

Conversation

4vtomat
Copy link
Member

@4vtomat 4vtomat commented Oct 8, 2024

We currently have over 67000 intrinsics, uint16_t will overflow.

@llvmbot llvmbot added clang Clang issues not falling into any other category backend:RISC-V clang:frontend Language frontend issues, e.g. anything involving "Sema" labels Oct 8, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 8, 2024

@llvm/pr-subscribers-clang

@llvm/pr-subscribers-backend-risc-v

Author: Brandon Wu (4vtomat)

Changes

We currently have over 67000 intrinsics, uint16_t will overflow.


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

1 Files Affected:

  • (modified) clang/lib/Sema/SemaRISCV.cpp (+1-1)
diff --git a/clang/lib/Sema/SemaRISCV.cpp b/clang/lib/Sema/SemaRISCV.cpp
index 3da4b515b1b114..f49c2088ef4776 100644
--- a/clang/lib/Sema/SemaRISCV.cpp
+++ b/clang/lib/Sema/SemaRISCV.cpp
@@ -399,7 +399,7 @@ void RISCVIntrinsicManagerImpl::InitRVVIntrinsic(
                                      Record.HasFRMRoundModeOp);
 
   // Put into IntrinsicList.
-  uint16_t Index = IntrinsicList.size();
+  uint32_t Index = IntrinsicList.size();
   assert(IntrinsicList.size() == (size_t)Index &&
          "Intrinsics indices overflow.");
   IntrinsicList.push_back({BuiltinName, Signature});

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

Why doesn't any test catch it?

@jerryzj
Copy link
Contributor

jerryzj commented Oct 8, 2024

Why doesn't any test catch it?

I suppose the CI of intrinsic doc should catch this, but it is not reporting errors as expected.
I happen to notice the failures and working on a fix now.

Copy link
Contributor

@wangpc-pp wangpc-pp left a comment

Choose a reason for hiding this comment

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

LGTM.

@topperc
Copy link
Collaborator

topperc commented Oct 8, 2024

Why don’t any of our lit tests that use every intrinsic catch it?

@@ -399,7 +399,7 @@ void RISCVIntrinsicManagerImpl::InitRVVIntrinsic(
Record.HasFRMRoundModeOp);

// Put into IntrinsicList.
uint16_t Index = IntrinsicList.size();
uint32_t Index = IntrinsicList.size();
Copy link
Collaborator

@topperc topperc Oct 8, 2024

Choose a reason for hiding this comment

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

What is the type of Intrinsics on this line further down

Intrinsics.insert({Name, Index});

is it also uint16_t?

@wangpc-pp
Copy link
Contributor

Why don’t any of our lit tests that use every intrinsic catch it?

We don't see any error, is it because your downstream added some intrinsics?

@topperc
Copy link
Collaborator

topperc commented Oct 8, 2024

Does the number intrinsics vary with the number of extensions enabled? Maybe we don't have a test with all extensions?

@4vtomat
Copy link
Member Author

4vtomat commented Oct 8, 2024

Why don’t any of our lit tests that use every intrinsic catch it?

Somehow if we add more "target-feature" the intrinsics increase, I'm not sure why.
For example:

test.c

#include "riscv_vector.h"
  vfloat16mf4x7_t test_vlseg7e16ff_v_f16mf4x7(const _Float16 *base, size_t *new_vl, size_t vl) {
  return __riscv_vlseg7e16ff_v_f16mf4x7(base, new_vl, vl);
}

Compile with this command:

clang -cc1 -triple riscv64-- -S -target-feature +v -target-feature +zvfh -target-abi lp64d test.c

generate 61674 intrinsics.
However if we add -target-feature +zvfbfmin

clang -cc1 -triple riscv64-- -S -target-feature +v -target-feature +zvfh -target-feature +zvfbfmin -target-abi lp64d test.c

generate 64568 intrinsics

it's weird.

@topperc
Copy link
Collaborator

topperc commented Oct 8, 2024

Why don’t any of our lit tests that use every intrinsic catch it?

Somehow if we add more "target-feature" the intrinsics increase, I'm not sure why. For example:

test.c

#include "riscv_vector.h"
  vfloat16mf4x7_t test_vlseg7e16ff_v_f16mf4x7(const _Float16 *base, size_t *new_vl, size_t vl) {
  return __riscv_vlseg7e16ff_v_f16mf4x7(base, new_vl, vl);
}

Compile with this command:

clang -cc1 -triple riscv64-- -S -target-feature +v -target-feature +zvfh -target-abi lp64d test.c

generate 61674 intrinsics. However if we add -target-feature +zvfbfmin

clang -cc1 -triple riscv64-- -S -target-feature +v -target-feature +zvfh -target-feature +zvfbfmin -target-abi lp64d test.c

generate 64568 intrinsics

it's weird.

Why is it weird? Some intrinsics require specific features. If that feature isn't enabled, the intrinsic is skipped.

@4vtomat
Copy link
Member Author

4vtomat commented Oct 8, 2024

Why don’t any of our lit tests that use every intrinsic catch it?

Somehow if we add more "target-feature" the intrinsics increase, I'm not sure why. For example:

test.c

#include "riscv_vector.h"
  vfloat16mf4x7_t test_vlseg7e16ff_v_f16mf4x7(const _Float16 *base, size_t *new_vl, size_t vl) {
  return __riscv_vlseg7e16ff_v_f16mf4x7(base, new_vl, vl);
}

Compile with this command:

clang -cc1 -triple riscv64-- -S -target-feature +v -target-feature +zvfh -target-abi lp64d test.c

generate 61674 intrinsics. However if we add -target-feature +zvfbfmin

clang -cc1 -triple riscv64-- -S -target-feature +v -target-feature +zvfh -target-feature +zvfbfmin -target-abi lp64d test.c

generate 64568 intrinsics

it's weird.

Why is it weird? Some intrinsics require specific features. If that feature isn't enabled, the intrinsic is skipped.

Oh I see, but if we extend the size, the memory usage will grow.

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

I think we need to change

struct RVVOverloadIntrinsicDef {
  // Indexes of RISCVIntrinsicManagerImpl::IntrinsicList.
  SmallVector<uint16_t, 8> Indexes;
};

and

  // Mapping function name to index of IntrinsicList.
  StringMap<uint16_t> Intrinsics;

Those places were both changed by e0092ea in addition to the place you've currently changed.

@4vtomat
Copy link
Member Author

4vtomat commented Oct 17, 2024

I think we need to change

struct RVVOverloadIntrinsicDef {
  // Indexes of RISCVIntrinsicManagerImpl::IntrinsicList.
  SmallVector<uint16_t, 8> Indexes;
};

and

  // Mapping function name to index of IntrinsicList.
  StringMap<uint16_t> Intrinsics;

Those places were both changed by e0092ea in addition to the place you've currently changed.

Oh, nice catch! I didn't even remember I've changed this before lol~

Copy link
Collaborator

@topperc topperc left a comment

Choose a reason for hiding this comment

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

LGTM

@goldsteinn
Copy link
Contributor

Can this get merged?

@topperc topperc merged commit e3b22dc into llvm:main Oct 18, 2024
8 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
backend:RISC-V clang:frontend Language frontend issues, e.g. anything involving "Sema" clang Clang issues not falling into any other category
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants