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

[Roger Ferrer Ibanez via Phabricator via llvm-commits]

rogfer01 added a comment.

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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92228