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

[RISCV] Add an implementation of findRepresentativeClass to assign i32 to GPRRegClass for RV64. #116165

Open
wants to merge 4 commits into
base: main
Choose a base branch
from

Conversation

topperc
Copy link
Collaborator

@topperc topperc commented Nov 14, 2024

This is an alternative fix for #81192. This allows the SelectionDAG scheduler to be able to find a representative register class for i32 on RV64. The representative register class is the super register class with the largest spill size that is also legal. The default implementation of findRepresentativeClass only works for legal types which i32 is not for RV64.

I wanted to remove i32 from the GPR register class to fix the issue, but we need to be able to write some i32 patterns for GISel. And now I need to add i16 to GPR for GISel.

I did some investigation of why tablegen uses i32 in output patterns on RV64. It appears it comes down to a function call ForceArbitraryInstResultType that just picks a type for the output pattern when the isel pattern isn't specific enough. I believe it picks the smallest type(lowested numbered) to resolve the conflict.

A similar issue occurs for f16 and bf16 which both use the FPR16 register class. If the isel pattern doesn't specify, tablegen may find both f16 bf16 and may pick bf16 from Zfh pattern when Zfbfmin isn't present. Since bf16 isn't legal in that case, findRepresentativeClass will fail.

This patch calls the base class with XLenVT to get the representative class since XLenVT is always legal.

For bf16/f16 we call the base class with f32 since all of the f16/bf16 extensions depend on either F or Zfinx which will make f32 a legal type. The final representative register class further depends on whether D or Zdinx is also enabled, but that should be handled by the default implementation.

…2 to GPRRegClass for RV64.

This is an alternative fix for llvm#81192. This allows the SelectionDAG
scheduler to be able to find a register class for i32 on RV64. The
default implementation of findRepresentativeClass only works for
legal types which i32 is not for RV64.

I wanted to remove i32 from the GPR register class to fix the issue,
but we need to be able to write some i32 patterns for GISel. And now
it looks like I need to add i16 to GPR. I had tried to use manual
instruction selection for some cases in GISel in llvm#116111, but I got
some feedback recommending the use of patterns.

I did some investigation of why tablegen uses i32 in output patterns
on RV64. It appears it comes down to ForceArbitraryInstResultType
that just picks a type for the output pattern when the isel pattern
isn't specific enough. I believe it picks the smallest type(lowested
numbered) to resolve the conflict.
@llvmbot
Copy link

llvmbot commented Nov 14, 2024

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

Author: Craig Topper (topperc)

Changes

This is an alternative fix for #81192. This allows the SelectionDAG scheduler to be able to find a register class for i32 on RV64. The default implementation of findRepresentativeClass only works for legal types which i32 is not for RV64.

I wanted to remove i32 from the GPR register class to fix the issue, but we need to be able to write some i32 patterns for GISel. And now it looks like I need to add i16 to GPR. I had tried to use manual instruction selection for some cases in GISel in #116111, but I got some feedback recommending the use of patterns.

I did some investigation of why tablegen uses i32 in output patterns on RV64. It appears it comes down to ForceArbitraryInstResultType that just picks a type for the output pattern when the isel pattern isn't specific enough. I believe it picks the smallest type(lowested numbered) to resolve the conflict.


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

2 Files Affected:

  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.cpp (+18)
  • (modified) llvm/lib/Target/RISCV/RISCVISelLowering.h (+3)
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
index 3df8eca8cae7fb..f191b4e9cb251e 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.cpp
@@ -21996,6 +21996,24 @@ SDValue RISCVTargetLowering::expandIndirectJTBranch(const SDLoc &dl,
   return TargetLowering::expandIndirectJTBranch(dl, Value, Addr, JTI, DAG);
 }
 
+// Some types are listed in the GPR register class to support isel patterns for
+// GISel, but are not legal in SelectionDAG. This prevents the default
+// implementation from finding a register clss for them.
+std::pair<const TargetRegisterClass *, uint8_t>
+RISCVTargetLowering::findRepresentativeClass(const TargetRegisterInfo *TRI,
+                                             MVT VT) const {
+  const TargetRegisterClass *RRC = nullptr;
+  uint8_t Cost = 1;
+  switch (VT.SimpleTy) {
+  default:
+    return TargetLowering::findRepresentativeClass(TRI, VT);
+  case MVT::i8: case MVT::i16: case MVT::i32:
+    RRC = &RISCV::GPRRegClass;
+    break;
+  }
+  return std::make_pair(RRC, Cost);
+}
+
 namespace llvm::RISCVVIntrinsicsTable {
 
 #define GET_RISCVVIntrinsicsTable_IMPL
diff --git a/llvm/lib/Target/RISCV/RISCVISelLowering.h b/llvm/lib/Target/RISCV/RISCVISelLowering.h
index 9ae70d257fa442..aa73d558c048b2 100644
--- a/llvm/lib/Target/RISCV/RISCVISelLowering.h
+++ b/llvm/lib/Target/RISCV/RISCVISelLowering.h
@@ -1051,6 +1051,9 @@ class RISCVTargetLowering : public TargetLowering {
 
   SDValue emitFlushICache(SelectionDAG &DAG, SDValue InChain, SDValue Start,
                           SDValue End, SDValue Flags, SDLoc DL) const;
+
+  std::pair<const TargetRegisterClass *, uint8_t>
+  findRepresentativeClass(const TargetRegisterInfo *TRI, MVT VT) const override;
 };
 
 namespace RISCVVIntrinsicsTable {

Copy link

github-actions bot commented Nov 14, 2024

✅ With the latest revision this PR passed the C/C++ code formatter.

topperc added a commit that referenced this pull request Nov 14, 2024
…biguity.

See also #81192. These were found by disabling tablegen's
ForceArbitraryInstResultType.

For one of the patterns I was able to get a failure if Zfh was enabled,
but Zfbfmin was not. It appears ForceArbitraryInstResultType picks
bf16 over f16.

I think something like #116165 is a better long term fix for these
issues. I will update that to include f16/bf16.
@topperc topperc marked this pull request as draft November 15, 2024 23:39
@topperc topperc marked this pull request as ready for review November 16, 2024 00:12
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants