This is an archive of the discontinued LLVM Phabricator instance.

[RISCV][RVV] Select unmasked TU RVV pseudos in a DAG post-process
ClosedPublic

Authored by arcbbb on Mar 16 2022, 11:47 PM.

Details

Summary

Following D118810 that reduced the size of ISel table,
this patch optimizes allone-masked RVV pseudos with TU policy and swap them
out to their unmasked TU pseudos.

Since the UNDEF merge operand is not preserved, we turn it into TA pseudo regardless of the policy operand.

Diff Detail

Event Timeline

arcbbb created this revision.Mar 16 2022, 11:47 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 16 2022, 11:47 PM
arcbbb requested review of this revision.Mar 16 2022, 11:47 PM
arcbbb retitled this revision from [RISCV][RVV] Select unmaksed TU RVV pseudos in a DAG post-process to [RISCV][RVV] Select unmasked TU RVV pseudos in a DAG post-process.Mar 16 2022, 11:53 PM
rogfer01 added inline comments.Mar 17 2022, 1:21 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2276

Sorry, I fail to follow the logic here. I'm sure I'm missing something very obvious.

My understanding is that the original code refrained itself from doing this optimization because it picks a pseudo that does not have a merge operand, so a tail policy that is not agnostic (i.e. tail undisturbed) is not eligible.

Your change allows picking a tail undisturbed pseudo when the tail policy is undisturbed (i.e. when the tail policy is not agnostic), why is then having an undef as a merge operand a problem?

In fact I think this is what @frasercrmck mentioned here https://reviews.llvm.org/D118810#3293768

One option, also mentioned by @frasercrmck in https://reviews.llvm.org/D118810#3296415, is to convert this specific case into a tail agnostic case, rather than giving up as your change does.

Does this make sense?

arcbbb added inline comments.Mar 17 2022, 3:17 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2276

Yes you are right, I think I am hesitant to make a decision whether to replace it with unmasked TA or unmasked TU.
Both seems reasonable to me, and besides I don't have a test case for that yet, maybe I should leave a FIXME here before we know how to handle it?

arcbbb updated this revision to Diff 416207.Mar 17 2022, 9:28 AM

Updates:

  1. Addresses @rogfer01's comment. Transforms TU+IMPLICIT_DEF to TA. This can be changed later if we have alternative idea.
  2. Adds TU+IMPLICIT_DEF test cases by
    sed -i '/; CHECK.*/d' $file
    sed -i -e '/define.*@intrinsic_vf.\?cvt_mask/{
           h;
           s/define <vscale.*> @intrinsic_\(vf.\?cvt\)_mask\(.*\)(\(<vscale.*>\) %0, \(<vscale.*>\) %1, <vscale x \(.*\) x i1> %2, iXLen %3) \(.*\)/\
define \3 @intrinsic_allone_\1_mask\2(\4 %1, iXLen %3) \6\
entry:\
  %allone = call <vscale x \5 x i1> @llvm.riscv.vmset.nxv\5i1(\
    iXLen %3);/;
           x;
           n;n;N;N; N;N;N;N;N;
           H;
           x;
           s/%0/undef/g
           s/%1/%0/g
           s/%2/%allone/g
           s/%3/%1/g
           s/iXLen 1/iXLen 0/g
           x;
           G;}
frasercrmck added inline comments.Mar 21 2022, 3:01 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2275

If it's TAIL_UNDISTURBED and the merge operand isn't undef, shouldn't we be bailing?

2305

Id isn't a good name: sounds to me like "identifier"

llvm/lib/Target/RISCV/RISCVISelLowering.cpp
4773 ↗(On Diff #416207)

Feels like this change should go into D120449?

khchen added inline comments.Mar 21 2022, 8:53 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2275

I think we had not discuss what's proper way to handle conflict values between merge and policy operand.

  1. merge operand is not a undef value but policy is agnostic. (ex. tama)
  2. merge operand is a undef value but policy is undisturbed. (ex. tamu/tuma/tumu)

In fact, I didn't have strong opinion here. so in https://reviews.llvm.org/D120226 patch, I changed VSETVLI inserter to update vtype according to the policy value directly rather than checking merge operand is undef or not to do something (ex. report an error, or auto fixup), because I think users have explicitly specific the policy value, we need to respect it.

auto fixup means backend could update the policy value for 1. with tumu and 2. with tama.

craig.topper added inline comments.Mar 21 2022, 9:05 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2275

From the register allocation standpoint, an undef operand gives the register allocator permission to pick whatever register is convenient. So it definitely won't preserve anything in a way a user could rely on. So in that case the policy is only going to affect the vsetlvli.

arcbbb updated this revision to Diff 417181.Mar 21 2022, 9:54 PM
arcbbb edited the summary of this revision. (Show Details)

Updates:

  1. Renames Id to I, and IsTU to IsTA
  2. Makes this patch independent of D120449.
arcbbb updated this revision to Diff 417185.Mar 21 2022, 9:59 PM

Fixed typo in the test filename.

arcbbb added inline comments.Mar 21 2022, 10:07 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2275

I discover that the inserted vsetvli uses TA for TU pseudos with IMPLICIT_DEF tie operand, so I follow the logic in computeInfoForInstr at RISCVInsertVSETVLI.cpp to transform it into TA pseudo.
Does this make sense?

arcbbb added inline comments.Apr 6 2022, 6:53 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2275

@frasercrmck Does this make sense to you?

arcbbb added inline comments.Apr 6 2022, 7:03 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
429

After D123217 there are cases where _TU pseudo doesn't exist.
I have to fix this logic.

arcbbb added inline comments.Apr 6 2022, 9:00 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2275

@frasercrmck Sorry for the noise. I didn't notice the enum was changed.

frasercrmck added inline comments.Apr 7 2022, 7:50 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2275

Sorry, I got a bit lost by the conversation and then it dropped off my radar. I think it's resolved though? Converting TU + IMPLICIT_DEF to TA makes sense to me.

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
429

I'm sorry about that. Can we make it optional somehow?

llvm/test/CodeGen/RISCV/rvv/allone-masked-to-unmasked.ll
1

Could these tests be pre-commited?

arcbbb updated this revision to Diff 421511.Apr 8 2022, 6:44 AM
arcbbb marked 7 inline comments as done.

Updates

  1. Add a placeholder for pseudos that don't have _TU variants.
arcbbb added inline comments.Apr 8 2022, 6:46 AM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
429

It just happened sooner than I thought it would be.
Not sure if I have done it right, I create a pseudo as a placeholder.

craig.topper added inline comments.Apr 12 2022, 10:39 AM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2263–2264

"we're expecting" here doesn't make sense does it? It's more like "but we might have a Glue operand last".

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
429

Can you use !if instead of !cond? Then you don't need the true : part

429

Instead of having a Dummy Pseudo, could you use the MaskedPseudo value. Then in RISCVISelDAGToDAG.cpp detect that the return value was the same opcode you started with?

arcbbb updated this revision to Diff 422393.Apr 12 2022, 9:43 PM
arcbbb marked 3 inline comments as done.
arcbbb edited the summary of this revision. (Show Details)

Address Craig's comments.

arcbbb added inline comments.Apr 12 2022, 9:44 PM
llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
429

It looks better now. Thanks!

I'm glad we don't need to add dummy pseudos for this to work :)

This is now looking good, thank you: just to check - there's no way to dynamically check whether the TU pseudo exists in TableGen? That would remove the need for HasTU which I'd find a bit cleaner.

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

This comment should probably be inverted. Something like "if the merge operand isn't IMPLICIT_DEF then we can't use TA"?

llvm/lib/Target/RISCV/RISCVInstrInfoVPseudos.td
427

Can get rid of this now it's unused?

arcbbb updated this revision to Diff 424937.Apr 25 2022, 9:27 AM

Address @frasercrmck 's comments

I'm glad we don't need to add dummy pseudos for this to work :)

This is now looking good, thank you: just to check - there's no way to dynamically check whether the TU pseudo exists in TableGen? That would remove the need for HasTU which I'd find a bit cleaner.

I read through the TableGen document several times for this, but it is a pity that I didn't discover any operator that can help.

craig.topper added inline comments.Apr 25 2022, 7:39 PM
llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp
2285

Could shorten the checks in the assert by doing

uint64_t TSFlags = TII->get(I->UnmaskedPseudo).TSFlags;
2295

Same here

llvm/lib/Target/RISCV/RISCVISelDAGToDAG.h
194

Probably should be UnmaskedTUPseudo, since TU is an abbreviation and not a word.

arcbbb updated this revision to Diff 425126.Apr 25 2022, 11:40 PM

Address Craig's comments.

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