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;

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;
}

@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

<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

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