[PATCH] D90973: [RISCV] Remove RV32 HwMode. Use DefaultMode for RV32

[Krzysztof Parzyszek via Phabricator via llvm-commits]

kparzysz added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCV.td:182-183
 
+// RV32 uses DefaultMode.
 def RV64           : HwMode<"+64bit">;
 
----------------
jrtc27 wrote:
> kparzysz wrote:
> > jrtc27 wrote:
> > > craig.topper wrote:
> > > > jrtc27 wrote:
> > > > > craig.topper wrote:
> > > > > > lenary wrote:
> > > > > > > lenary wrote:
> > > > > > > > Is there a way of doing an alias here, so we don't have to document that defaultmode is rv32 everywhere?
> > > > > > > > 
> > > > > > > > Something like the following (I guess it has to be a let, not a def, but I could be wrong).
> > > > > > > > ```
> > > > > > > > let RV32 = DefaultMode;`
> > > > > > > > ```
> > > > > > > Sorry, it can't be a let, but you could do the following, I think
> > > > > > > ```
> > > > > > > defvar RV32 = DefaultMode;
> > > > > > > ```
> > > > > > > 
> > > > > > > And then in an example pulled from below:
> > > > > > > ```
> > > > > > > def XLenVT : ValueTypeByHwMode<[RV64, RV32],
> > > > > > >                                [i64,  i32]>;
> > > > > > > ```
> > > > > > That works, so I'll switch to that.
> > > > > Hm, does this work the other way round too (and if not, what happens)? Having RV64 before RV32 is not the usual order.
> > > > It does! Switched to that order in 9211da4215b6d48c8b9186b78274946789c559e9
> > > Thanks! I always expect the worst when HwMode is involved...
> > > I always expect the worst when HwMode is involved...
> > 
> > What's causing this?  Is there anything specific that could be improved?
> The main issues I see with HwMode are that, unless I am mistaken, all HwMode instances must be mutually-exclusive such that if you have two orthogonal modes you need to take the cross product of them manually, which is particularly problematic for downstream as that can cause significant diffs. Ideally you would be able to have separate "groups" of HwMode that are internally mutually-exclusive and TableGen could do the cross-product for you (or maybe something smarter so as to not bloat the generated code), or failing that you'd have to define the exploded list but then be able to define aliases as sets of HwMode's.
> 
> As a more minor thing there's also the fact that only a very limited set of fields can be HwMode-dependent, and at least downstream it would be convenient to have mode-dependent sub-register definitions; currently we just use `SubRegIndex<-1, -1>`.
The mutual exclusivity requirement is true, and there are a few reasons for that.  The initial reason was ensuring deterministic behavior during instruction selection: selection patterns are applied in some order, and if one pattern fails, the next one is applied.  Since `HwMode` expands into predicates, a single source code pattern becomes multiple ones in the matching code.  There is no natural ordering between such clones that would be clear at the source code level, so with the "first matched, first serve" approach, you'd get unexpected behavior.

The HwMode was designed to be a single parameter that describes (enumerates) the configuration of the hardware.  Historically, the initial motivation was handling 64- and 128-byte vectors on Hexagon with the same instruction set, then the 32- and 64-bit modes of RISCV became another potential user.  Keeping other targets unaffected (i.e. not forcing them to change anything in their .td files) was a very significant factor in the design, or else this feature would have little chance of making it in.

Having aliases for sets of modes may actually not be hard to add.  Could you show me an example of how you'd use it?


The way that TableGen handles registers and subregisters doesn't make things easy to customize.  If I wanted to add parameterized subregister definitions, it would have likely affected the lane mask calculations, and have a chain of (unnecessarily desirable) side-effects that could be difficult to track down.  The lack of handling subregisters is actually the part of the HwMode feature that I dislike, but I'm afraid it would take a while to implement it properly.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D90973