This is an archive of the discontinued LLVM Phabricator instance.

[RISCV] Add passthru operand for RVV nomask load intrinsics.
ClosedPublic

Authored by khchen on Jan 19 2022, 1:30 AM.

Details

Summary

The goal is support tail and mask policy in RVV builtins.
We focus on IR part first.
If the passthru is undef, we use tail agnostic, otherwise
use tail undisturbed.

Co-Authored-by: Hsiangkai Wang <Hsiangkai@gmail.com>

Diff Detail

Event Timeline

khchen created this revision.Jan 19 2022, 1:30 AM
khchen requested review of this revision.Jan 19 2022, 1:30 AM
Herald added projects: Restricted Project, Restricted Project. · View Herald TranscriptJan 19 2022, 1:30 AM
craig.topper added inline comments.Jan 19 2022, 3:30 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
1267
bool IsTU = IntNo != Intrinsic::riscv_vlm && (!Node->getOperand(CurOp).isUndef() || IsMasked);
NOTE: I changed it so IsMask is also only checked when it's not vlm. That makes the non vlm part of the condition look like the other intrinsic handling earlier.
1281

Why aren't we using the default value for IndexVT now?

khchen updated this revision to Diff 401446.Jan 19 2022, 4:46 PM

Address Craig's comments, thanks!

Is "destination operand" the terminology we want? I'd have thought "passthru" was more conventional.

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
1222

If I'm reading this right, isn't it possible that we could have a masked/tu intrinsic, so push back the passthru operand, increment CurOp then check whether the pointer operand is Undef, then wrongly increment CurOp a second time? So basically an undef base pointer has the potential to (presumably) crash the compiler somewhere later?

Don't we unconditionally want to increment CurOp (a single time) since we now always have a passthru operand that we want to skip over?

khchen updated this revision to Diff 401899.Jan 21 2022, 1:32 AM

address frasercrmck's comments.
rewrite snippet of code to fix potential bug and make more readable.

Thanks, LGTM.

As a heads up, I've pinched the use of _TU as a suffix in D117561. The conflicts should be minor (one location) for whichever patch is merged second.

khchen retitled this revision from [RISCV] Add destination operand for RVV nomask load intrinsics. to [RISCV] Add passthru operand for RVV nomask load intrinsics..Jan 21 2022, 8:01 AM
khchen edited the summary of this revision. (Show Details)
khchen updated this revision to Diff 402776.Jan 25 2022, 12:11 AM

rebase and ping.

This revision is now accepted and ready to land.Jan 25 2022, 8:43 AM
This revision was landed with ongoing or failed builds.Jan 25 2022, 5:36 PM
This revision was automatically updated to reflect the committed changes.