[PATCH] D124869: [RISCV] Hoist VSETVLI out of idiomatic fixed length vector loops

[Philip Reames via Phabricator via llvm-commits]

reames added inline comments.


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:1293
+
+  if (RISCVII::LMUL_1 != Info.getVLMUL())
+    return false;
----------------
craig.topper wrote:
> What prevents supporting other LMULs?
Nothing, we just need to adjust the formula below for it.  Mind if I do this in a separate change?  I'm still not 100% trusting my mental model of L:MUL, and would prefer to have that reviewed on its own.  

I will add a TODO.  


================
Comment at: llvm/lib/Target/RISCV/RISCVInsertVSETVLI.cpp:105
+
+  uint8_t getSEW() const { return SEW; }
+  RISCVII::VLMUL getVLMUL() const { return VLMul; }
----------------
craig.topper wrote:
> reames wrote:
> > craig.topper wrote:
> > > Any reason to restrict this uint8_t? Looks like it's a assigned to unsigned where it's used.
> > uint8_t matches the actual type of the field.  I can change it if you want, but it seems cleaner to me to match the field type.  
> I was thinking of the uint8_t as an implementation choice I made to keep the data structure size down. It could have been a 7 bit bitfield or stored in log2 form. It would also need to change if SEW=256, 512, or 1024 came back to life.
> 
> That was just my thinking. If you want to keep it as uint8_t it's fine with me.
I see your point, could go either way.  I think either answer is reasonable here.  


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

https://reviews.llvm.org/D124869