[PATCH] D103510: [RISCV] Use vmv.v.[v|i] if we know COPY is under the same vl and vtype.

Tue Oct 26 06:38:14 PDT 2021

frasercrmck accepted this revision.
frasercrmck added a comment.
This revision is now accepted and ready to land.

In D103510#3080823 <https://reviews.llvm.org/D103510#3080823>, @HsiangKai wrote:

> In D103510#3077278 <https://reviews.llvm.org/D103510#3077278>, @frasercrmck wrote:
>
>> Sorry for being sluggish on the review. I'm okay with the idea and the approach but I wouldn't have caught the various issues which seem to have cropped up during review, like those with widening reductions or zvlsseg tuples, etc. I think that's why I'm hesitant to give it an official approval. I'm not sure how to take it forward though. Presumably you're using this pass by default in your downstream fork?
>>
>> I can say that I have taken this patch and run some of our testing with it. Everything seemed fine, though I realise that's not formal verification.
>
> Sorry for replying late. Yes, we enable the conversion by default in our downstream. We have a random test generator to run thousands and thousands tests on it. We found several bugs in my initial version and I have applied these fixes on this patch. You could see these cases from the test case in this patch. How about to turn it off by default on the upstream?

No I think that's fine. I've given it another look over and it LGTM aside from my last comment. Thanks for your patience!

================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfo.cpp:204
+
+      for (const MachineOperand &MO : MBBI->defs()) {
+        if (!FoundDef && TRI->isSubRegisterEq(MO.getReg(), SrcReg)) {
----------------
HsiangKai wrote:
> frasercrmck wrote:
> > Do we need to worry about implicit defs?
> The function only handles vector COPY. Is there an instruction to have an implicit define for vector registers?
I'm not aware of one, but I still think it's more theoretically sound to check all defs in the instruction. I think we agree that if there was such an implicit def we'd need to handle it? Therefore at lesat an assert that we're not expecting that case.

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D103510/new/

https://reviews.llvm.org/D103510