[PATCH] D109963: [AArch64] Split bitmask immediate of bitwise AND operation

[JinGu Kang via Phabricator via llvm-commits]

jaykang10 added inline comments.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:52
+  void getAnalysisUsage(AnalysisUsage &AU) const override {
+    AU.addRequired<MachineLoopInfo>();
+    MachineFunctionPass::getAnalysisUsage(AU);
----------------
dmgreen wrote:
> Can this preserve CFG, which should help it preserve some analysis passes
Yep, I think so. Let me add `AU.setPreservesCFG`.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:66
+bool AArch64MIPeepholeOpt::visitAND(
+    MachineInstr &MI, unsigned regSize,
+    SmallVectorImpl<MachineInstr *> &ToBeRemoved) {
----------------
dmgreen wrote:
> Can this use regsize == sizeof(T), to simplify it a little
Yep, let me update it.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:81
+  MachineLoop *L = MLI->getLoopFor(MBB);
+  if (L && !L->isLoopInvariant(MI))
+    return false;
----------------
dmgreen wrote:
> Does it matter if MI is loop invariant? As opposed to the MOV.
Let's say `MOV` is in preheader and `AND` is in the loop as below.
```
preheader:
  %1:gpr32 = MOVi32imm 2098176
  %2:gpr32 =
...
loop:
... 
  %3:gpr32 = ANDWrr killed %2:gpr32, %1:gpr32
```
In above example, AND is loop invariant and we can split the constant of MOV.
```
preheader:
  %1:gpr32 = MOVi32imm 2098176
...
loop:
...
  %2:gpr32 = PHI 
  %3:gpr32 = ANDWrr killed %2:gpr32, %1:gpr32
```
In above example, the AND is not loop invariant and we can not split the constant of MOV because the first AND needs %2 and the AND is outside loop.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:129
+  // If mov instruction has one use, delete it.
+  if (MRI->hasOneUse(DefMI->getOperand(0).getReg()))
+    ToBeRemoved.push_back(DefMI);
----------------
dmgreen wrote:
> Should this transform be done with multiple uses on the mov?
I thought there could be other opportunities with two bitmask immediate and other bitwise operations like `OR`. If possible, I would like to keep the MOV for the potential opportunities.


================
Comment at: llvm/lib/Target/AArch64/MCTargetDesc/AArch64AddressingModes.h:342
+template <typename T>
+static inline bool isValidAndSplitBitmaskImm(T Imm, unsigned regSize) {
+  assert((regSize == 32 || regSize == 64) &&
----------------
dmgreen wrote:
> This can move into the peephole optimization pass now, which might allow it to be simplified somewhat.
This function could be used on other places. If possible, I would like to keep this check here conservatively.


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

https://reviews.llvm.org/D109963