[PATCH] D26577: [AVR] Add the pseudo instruction expansion pass

[Krzysztof Parzyszek via llvm-commits]

kparzysz added inline comments.


================
Comment at: lib/Target/AVR/AVRExpandPseudoInsts.cpp:113
+
+  for(Block &MBB : MF) {
+    bool ContinueExpanding = true;
----------------
Space between "for" and "(".


================
Comment at: lib/Target/AVR/AVRExpandPseudoInsts.cpp:126
+      ContinueExpanding = BlockModified;
+    } while(ContinueExpanding);
+  }
----------------
Space after "while".


================
Comment at: lib/Target/AVR/AVRExpandPseudoInsts.cpp:155
+
+  if (ImpIsDead) { MIBHI->getOperand(3).setIsDead(); }
+
----------------
This formatting occurs in several places---please expand it into separate lines.


================
Comment at: lib/Target/AVR/AVRExpandPseudoInsts.cpp:255
+
+  auto MIBLO = buildMI(MBB, MBBI, OpLo)
+    .addReg(DstLoReg, RegState::Define | getDeadRegState(DstIsDead))
----------------
You can just use the actual opcode here (and below, for OpHi), since you are expanding a specific opcode.


================
Comment at: lib/Target/AVR/AVRExpandPseudoInsts.cpp:696
+template<typename Func>
+bool AVRExpandPseudo::expandAtomicBinaryOp(unsigned Opcode, Block &MBB, BlockIt MBBI, Func f) {
+  return expandAtomic(MBB, MBBI, [&](MachineInstr &MI) {
----------------
This line is too long.


================
Comment at: lib/Target/AVR/AVRExpandPseudoInsts.cpp:701
+
+      MachineInstr &NewInst = *buildMI(MBB, MBBI, Opcode).addOperand(Op1).addOperand(Op2).getInstr();
+
----------------
Same here.


================
Comment at: lib/Target/AVR/AVRExpandPseudoInsts.cpp:727
+      // Create the arithmetic op
+      buildMI(MBB, MBBI, ArithOpcode).addOperand(Op1).addOperand(Op1).addOperand(Op2);
+
----------------
Please limit lines to 80 columns.


================
Comment at: lib/Target/AVR/AVRExpandPseudoInsts.cpp:1147
+bool AVRExpandPseudo::expand<AVR::RORWRd>(Block &MBB, BlockIt MBBI) {
+  assert(0 && "RORW unimplemented");
+  return false;
----------------
Please use llvm_unreachable instead of assert(0).


https://reviews.llvm.org/D26577