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

[Manolis Tsamis via Phabricator via llvm-commits]

mtsamis marked 4 inline comments as done.
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:
> > 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.
Ok, I implemented it with a MaxShiftAmount argument that is exposed as a templated argument to the .td file.
Also restricted the re-arranged constant to be a simm12 one.

New test functions have been added for both.


================
Comment at: llvm/lib/Target/RISCV/RISCVInstrInfoXTHead.td:689
+  ValueType vt = XLenVT> {
+def : Pat<(st (vt GPR:$rd), GPR:$rs1, simm5:$off),
+          (Inst GPR:$rd, GPR:$rs1, simm5:$off, 0)>;
----------------
mtsamis wrote:
> craig.topper wrote:
> > We might look at a ComplexPattern here to match a shifted simm5 and return the simm5 and the shift amount as 2 results.
> Sounds good, I'll try to refactor that with a ComplexPattern.
Changed to a simm5shl2 complex pattern which makes the StoreUpdate patterns much more readable. 

Unfortunately the same pattern didn't fit for use in tryIndexedLoad because there is a potential negation of the offset and that made the code messier when the same pattern was used there.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D144249