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

[Craig Topper via Phabricator via llvm-commits]

craig.topper added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVISelDAGToDAG.cpp:2224
+      Index = N.getOperand(0);
+      ShiftAmt = N.getConstantOperandVal(1);
+    } else {
----------------
mtsamis wrote:
> 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.
I don't know of a way to do that from tablegen with the ComplexPattern. Tablegen would normally emit the check when it visited the shl. But since the shl is visited inside the complex pattern, the tablegen generated checks never see it and no checks are generated after the ComplexPattern is called.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144249