[PATCH] D92228: [RISCV] Add MIR tests exposing missed InstAliases

[Fraser Cormack via Phabricator via llvm-commits]

frasercrmck added a comment.

In D92228#2424007 <https://reviews.llvm.org/D92228#2424007>, @rogfer01 wrote:

> Hi all,
>
> a couple of comments.
>
> 1. First regarding the non-matched aliases: we also observed this issue when working on that and we deployed a bit of a hack. We added a new attribute `AllowsNoRegister`
>
>   // AllowsNoRegister - Specify that instructions may use NoRegister (0) instead
>   // of a physical register for an operand of this register class.
>   bit AllowsNoRegister = 0;
>
> And then we extended `VMV0` register class with this attribute
>
>   def VMV0 : RegisterClass<"RISCV", VMaskVT.types, 64, (add V0)> {
>     let Size = 64;
>     let AllowsNoRegister = 1;
>   }
>
> and then we updated `matchAliasCondition` in `MCInstPrinter.cpp` to look like this (after having implemented some needed plumbing to bring the bit where used)
>
>   case AliasPatternCond::K_RegClass:
>     // Operand must be a register in this class. Value is a register class id. 
>     return Opnd.isReg() && (MRI.getRegClass(C.Value).contains(Opnd.getReg()) ||
>                             (MRI.getRegClass(C.Value).allowsNoRegister() &&    
>                              Opnd.getReg() == 0));
>
> but this might feel a bit hackish, so I'm not suggesting this approach (unless there is a general sentiment that it is sensible).
>
> 2. Regarding `@earlyclobber`, yes it is not ideal as it is now. But it doesn't only impact masking (though perhaps that is the clearest case).
>
> Unless the ISA has changed (and I admit I might be a bit outdated here) my understanding is that we still have cases where `@earlyclobber` will be needed and it will still fall short.
>
> For instance, mixed widenings such as `vwadd.wv` are complex to model because iirc an instruction like `vwadd.wv v2, v2, v1` is fine while `vwadd.wv v2, v2, v3` is not. I understand that using `@earlyclobber` a thing like `vwadd.wv v4, v2, v1` is the best we can get.
>
> I asked about this here http://lists.llvm.org/pipermail/llvm-dev/2020-May/141383.html
>
> One of the suggestions made in that thread is teaching RA new tricks, which I don't feel qualified to do :) (I envision that if RA were able to dynamically limit the valid registers, as it assigns registers to operands, by virtue of some target-specific constraints, then, perhaps, this would be doable. But tbh I have no idea such approach is even feasible in the current RA. If we could do that we could also solve the case where `v0` can't be used as the destination of masked instructions).
>
> The other suggestion is to amend the instructions after RA in a specific pass. Seems doable at first but I am afraid this is going to be testing hell just to make sure no instruction manages to escape from that pass incorrectly.
>
> I personally don't oppose duplicating all the instructions between mask and unmasked if this simplifies things. Just let's be conscious that, for instructions with a mask, we would go from 7 pseudos per actual instruction to 14. Perhaps the impact is not that noticeable.

Thanks for your input, Roger. I think we can defer any work on the `InstAliases` until we know what we're doing about masked/unmasked operations. If we duplicate the instructions we're going to lose the only motivation (that I know of) to fix this in the `InstAlias` support.

Does this question affect the scope of D89449 <https://reviews.llvm.org/D89449> at all, or shall we address that question later?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92228