[PATCH] D47857: [RISCV] Add machine function pass to merge base + offset

[Sameer AbuAsal via Phabricator via llvm-commits]

sabuasal added inline comments.


================
Comment at: lib/Target/RISCV/RISCVMergeBaseOffset.cpp:1
+//== RISCVMergeBaseOffset.cpp - Fold offset into address lowering sequence ==//
+//                     The LLVM Compiler Infrastructure
----------------
asb wrote:
> asb wrote:
> > This line is only 79 chars long. Probably nicer to keep the usual format (three =) and have a shorter description.
> > 
> > e.g. `//== RISCVMergeBaseOffset.cpp - Fold offset into address lowering sequence ==//`
> Oops, wrong paste in that comment. Corrected:
> 
> `//===-- RISCVMergeBaseOffset.cpp - Optimise address calculations  ---------===//`
I actually spent a long time pondering the cleanest way to add this comment, yours is better so I'l use it :)


================
Comment at: lib/Target/RISCV/RISCVMergeBaseOffset.cpp:28-38
+#include "RISCVTargetObjectFile.h"
+#include "llvm/ADT/STLExtras.h"
+#include "llvm/CodeGen/Passes.h"
+#include "llvm/CodeGen/TargetLoweringObjectFileImpl.h"
+#include "llvm/CodeGen/TargetPassConfig.h"
+#include "llvm/IR/LegacyPassManager.h"
+#include "llvm/Support/Debug.h"
----------------
asb wrote:
> A number of these includes seem totally unused. e.g. STLExtras., RISCVTargetObjctFile.h, TargetLoweringObjectFileIMpl.h
Thanks for the comment, yeah I believe I need a second look here


================
Comment at: lib/Target/RISCV/RISCVMergeBaseOffset.cpp:143
+  assert((TailAdd.getOpcode() == RISCV::ADD) && "Expected ADD instruction!");
+  for (MachineOperand &Op : TailAdd.operands()) {
+    if (Op.isDef())
----------------
asb wrote:
> Given that we know the MI is an Add, is looping over the operands the clearest way of handling this?
I need to find the input argument that is coming from the offset creation; which can be a LUI or ADDI followed by LUI as explained in the comment above the function. 

Do you think there is a cleaner way to do it?


https://reviews.llvm.org/D47857