[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 {
----------------
mtsamis wrote:
> 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.
After adding additional tests, I found that there is a similar issue if the ADDI offset matched is large. I will also see how to properly address that in the AddrRegRegScale complex pattern.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144249