[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:85
+  // Check whether AND's operand is MOV with immediate.
+  MachineInstr *DefMI = MRI->getUniqueVRegDef(MI.getOperand(2).getReg());
+  MachineInstr *SubregToRegMI = nullptr;
----------------
dmgreen wrote:
> Consider renaming DefMI to MovMI, to clearly specify what it is expected to contain.
Yep, let me update it.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:103
+
+  // Check whether the bitmask immedaite can be split.
+  T UImm = static_cast<T>(Imm);
----------------
dmgreen wrote:
> Typo in immediate
Sorry, let me update it.


================
Comment at: llvm/lib/Target/AArch64/AArch64MIPeepholeOpt.cpp:108
+
+  // Split the bitmask immeidate into two.
+  T Imm1Enc = AArch64_AM::splitAndBitmaskImm(UImm, RegSize, true);
----------------
dmgreen wrote:
> A different typo in immediate :)
Sorry, again... Let me update it.


================
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:
> jaykang10 wrote:
> > 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.
> Yeah that might well happen. Do you have any tests? Maybe in an unrolled loop?
I do not have test cases yet. I could try to split the bitmask more later.


================
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:
> jaykang10 wrote:
> > 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.
> What other places do you expect it to be used?
> 
> It seems at the moment, with the way these are called they will be re-calculating the same values repeatedly. That was unavoidable from tablegen patterns, but shouldn't be necessary from the new Peephole pass, hopefully.
You are right! Let me move it to the peephole pass.


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

https://reviews.llvm.org/D109963