Skip to content

Conversation

jacquesguan
Copy link
Contributor

This pr removes the falling back to SDISel of rvv intrinsics and marks them legalized in the legalize pass. Another pr would be created for regbankselect pass to make vf form intriniscs have the right scalar register bank.

This pr removes the falling back to SDISel of rvv intrinsics and marks them legalized in the legalize pass.
Copy link

github-actions bot commented Sep 2, 2025

⚠️ undef deprecator found issues in your code. ⚠️

You can test this locally with the following command:
git diff -U0 --pickaxe-regex -S '([^a-zA-Z0-9#_-]undef[^a-zA-Z0-9_-]|UndefValue::get)' 'HEAD~1' HEAD llvm/test/CodeGen/RISCV/GlobalISel/rvv/vfadd.ll llvm/lib/Target/RISCV/GISel/RISCVLegalizerInfo.cpp llvm/lib/Target/RISCV/RISCVISelLowering.cpp llvm/test/CodeGen/RISCV/GlobalISel/irtranslator/fallback.ll

The following files introduce new uses of undef:

  • llvm/test/CodeGen/RISCV/GlobalISel/rvv/vfadd.ll

Undef is now deprecated and should only be used in the rare cases where no replacement is possible. For example, a load of uninitialized memory yields undef. You should use poison values for placeholders instead.

In tests, avoid using undef and having tests that trigger undefined behavior. If you need an operand with some unimportant value, you can add a new argument to the function and use that instead.

For example, this is considered a bad practice:

define void @fn() {
  ...
  br i1 undef, ...
}

Please use the following instead:

define void @fn(i1 %cond) {
  ...
  br i1 %cond, ...
}

Please refer to the Undefined Behavior Manual for more information.

Comment on lines 711 to 716
const RISCVVIntrinsicsTable::RISCVVIntrinsicInfo *II =
RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(IntrinsicID);

if (II) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

If II is otherwise unused, this should be:

Suggested change
const RISCVVIntrinsicsTable::RISCVVIntrinsicInfo *II =
RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(IntrinsicID);
if (II) {
return true;
}
if (RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(IntrinsicID))
return true;

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed, I was thinking to use the intrinsic info to identify which rvv intrinsics need specific legalization, since the change is not in this pr, I changed this to the suggested form.

Comment on lines 24703 to 24713
if (Op == Instruction::Call) {
const CallInst &CI = cast<CallInst>(Inst);
const Function *F = CI.getCalledFunction();
Intrinsic::ID ID = F ? F->getIntrinsicID() : Intrinsic::not_intrinsic;

const RISCVVIntrinsicsTable::RISCVVIntrinsicInfo *II =
RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(ID);
// Mark RVV intrinsic is supported.
if (II)
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
if (Op == Instruction::Call) {
const CallInst &CI = cast<CallInst>(Inst);
const Function *F = CI.getCalledFunction();
Intrinsic::ID ID = F ? F->getIntrinsicID() : Intrinsic::not_intrinsic;
const RISCVVIntrinsicsTable::RISCVVIntrinsicInfo *II =
RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(ID);
// Mark RVV intrinsic is supported.
if (II)
return false;
}
if (auto *II = dyn_cast<IntrinsicInst>(&Inst)) {
// Mark RVV intrinsic as supported.
if (RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(II->getIntrinsicID()))
return false;
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Addressed.

@topperc
Copy link
Collaborator

topperc commented Sep 2, 2025

Another pr would be created for regbankselect pass to make vf form intriniscs have the right scalar register bank.

What about i64 scalar operands on RV32?

; RUN: sed 's/iXLen/i64/g' %s | llc -mtriple=riscv64 -mattr=+v,+zfhmin,+zvfh \
; RUN: -verify-machineinstrs -target-abi=lp64d -global-isel | FileCheck %s

declare <vscale x 1 x half> @llvm.riscv.vfadd.nxv1f16.nxv1f16(
Copy link
Member

Choose a reason for hiding this comment

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

we no longer need to declare intrinsics

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks to point this, this test case is just copied from the SDISel case, maybe we need to refactor them all.

<vscale x 1 x half>,
iXLen, iXLen);

define <vscale x 1 x half> @intrinsic_vfadd_vv_nxv1f16_nxv1f16_nxv1f16(<vscale x 1 x half> %0, <vscale x 1 x half> %1, iXLen %2) nounwind {
Copy link
Member

Choose a reason for hiding this comment

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

nit: for most of the intrinsics here, their operands should have the same type, so a function like intrinsic_vfadd_vv_nxv1f16 should suffice

Copy link

github-actions bot commented Sep 3, 2025

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

@jacquesguan
Copy link
Contributor Author

Another pr would be created for regbankselect pass to make vf form intriniscs have the right scalar register bank.

What about i64 scalar operands on RV32?

Not only the i64 scalar operands on RV32, all integer scalars that is not XLen bits should be legalized. I plan to implement these in the next pr to support vx and vf form intrinsics.

Comment on lines +711 to +713
if (RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(IntrinsicID)) {
return true;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

nit: llvm style prefers simple stmt bodies don't have {}s

Suggested change
if (RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(IntrinsicID)) {
return true;
}
if (RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(IntrinsicID))
return true;

// Mark RVV intrinsic as supported.
if (RISCVVIntrinsicsTable::getRISCVVIntrinsicInfo(II->getIntrinsicID()))
return false;
}
Copy link
Contributor

Choose a reason for hiding this comment

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

okay to keep them here though, because of the comment.

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

Successfully merging this pull request may close these issues.

4 participants