[PATCH] D144249: [RISCV] Add vendor-defined XTheadMemIdx (Indexed Memory Operations) extension

[Manolis Tsamis via Phabricator via llvm-commits]

mtsamis added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2224
+      Index = N.getOperand(0);
+      ShiftAmt = N.getConstantOperandVal(1);
+    } else {
----------------
craig.topper wrote:
> mtsamis wrote:
> > craig.topper wrote:
> > > Do we need to check the shift amount is 1,2, or 3?
> > I wanted to make this pattern generic and not limited to specific shift amounts.
> > For the specific MemIdx extension the shift amount should be enforced on the pattern side with `(AddrRegRegScale GPR:$rs1, GPR:$rs2, uimm2:$uimm2)`.
> > If I'm not mistaken, SelectAddrRegRegScale can match any shift amount but that will be rejected by the uimm2 there.
> I worry the ComplexPattern might have bypassed that check.
I just checked and what you say is true (i.e. if SelectAddrRegRegScale returns value not in the 0-3 range the compiler crashes with 'Bad machine code: Invalid immediate').

Then, is there a way for the ComplexPattern to take into account the restrictions of the matched types (uimm2 in this case)?
I could add a template argument that explicitly sets the range accepted for the shift (and then have `... (AddrRegRegScale<4> ...)`) but I wanted something more elegant that doesn't require this. 
Are you aware of other ways to implement that restriction?

P.S. I will make sure to add testcases for that.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144249