[PATCH] D92105: [RISCV] Add pre-emit pass to make more instructions compressible

[Lewis Revill via Phabricator via llvm-commits]

lewis-revill added a comment.

In D92105#3474670 <https://reviews.llvm.org/D92105#3474670>, @reames wrote:

> Drive by review.  I'm mostly using this as an exercise to get familiar with the code and the target, so please don't consider any of my comments blocking.

Thanks very much for the review



================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:100
+// Return log2(widthInBytes) of load/store done by Opcode.
+static unsigned log2LdstWidth(unsigned Opcode) {
+  switch (Opcode) {
----------------
reames wrote:
> I would expect us to have a means for querying the width of a load/store somewhere, and for this to simply be the log of an existing utility.  I haven't dug around, but are you sure that doesn't exist?
I've had a brief look, and it seems to me the only existing way would be to use `MI.memoperands_begin()->getSize()`. Doesn't seem to be anything simple in `TargetInstrInfo.h` that we can use/override either to get this functionality.


================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:126
+static bool compressibleSPOffset(int64_t Offset, unsigned Opcode) {
+  return log2LdstWidth(Opcode) == 2 ? isShiftedUInt<6, 2>(Offset)
+                                    : isShiftedUInt<6, 3>(Offset);
----------------
reames wrote:
> Don't we have a means already for computing max offset representable in the addressing of an instruction?  I'd expect us to,  
No, every place which would need to check this just repeats the same logic since it's so simple.


================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:305
+      if (MO.isDef()) {
+        assert(isCompressibleLoad(MI));
+        continue;
----------------
reames wrote:
> It's not clear me to me why the last instruction has to be a load as this assert would seem to imply.  Could we instead directly assert this is the last instruction somehow?
We only look at loads and stores for compressibility, and as such if we see a compressible instruction which also defines the register we know it is a compressible load. We also know that since we stop when we see the next definition of the register we are looking at, but only after we've checked the defining instruction for compressibility. So the only way we are looking at a definition is if it's the final instruction.


================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:320
+  // This is a size optimization.
+  if (skipFunction(Fn.getFunction()) || !Fn.getFunction().hasMinSize())
+    return false;
----------------
reames wrote:
> While this is a size opt, I'd expect improving code density to outweigh the effect of an extra register move and/or add.  We should measure, but this might be useful even for O2.  
Until recently this was enabled at `-Os` but changed to `-Oz` due to the increase in number of instructions executed. I suggest keeping at `-Oz` and changing this later if there is a motivation for it.


================
Comment at: llvm/lib/Target/RISCV/RISCVMakeCompressible.cpp:343
+      SmallVector<MachineInstr *, 8> MIs;
+      Register NewReg = analyzeCompressibleUses(MI, RegImm, MIs);
+      if (!NewReg)
----------------
reames wrote:
> The algorithm here appears to be O(N^2).  Can we do better here?
Hmm, I guess there'd be a way to accumulate the ranges that we're interested in as we go through in one iteration - though I think this would be far more difficult to implement and you might end up with issues to do with looking at instructions which are yet to be updated and making incorrect conclusions (I don't see any cause for that at a first glance but it's always possible).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D92105