[RISCV] Add asserts that we don't increase codesize during pseudo expansion

[reames]

Summary

We currently assume that pseudo expansion doesn't increase the distance between a branch and it's label. This patch adds some asserts to catch violations of this property in pseudo expansion.

I chose to only do the assertion at the function level as we have to scan the whole function for size changes (since expansion can create multiple blocks). We could cache individual block sizes, and thus make the check cheap enough to do after every expansion, but that requires more complex code.

It also looks like we have some shared code structure here. If reviewers want, I'm happy to factor out a base class for pseudo expansion, and add the asserts there. I'm not sure this is worth doing, but will happily defer to reviewer preference.

[jrtc27]

s/macro/pseudo/? I got very confused until I read the diff and thought you were talking about assembly macros.

This is just checking that the Size of the pseudo is a true upper bound, right?

[reames]

In D142777#4086844, @jrtc27 wrote:

s/macro/pseudo/? I got very confused until I read the diff and thought you were talking about assembly macros.

This is just checking that the Size of the pseudo is a true upper bound, right?

Yes to both. Sorry for the confusion, fixed the patch description.

[craig.topper]

llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp

89

Will this warn in release builds? Should we put #ifndef NDEBUG around the loops?

89

Warn for unused variables I mean.

[reames]

llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp

89

It might since the variable is repeatedly increment, but your point is solid and deserves a rework regardless.

[luismarques]

Overall seems a reasonable implementation, despite the limitations pointed out in the patch description.

BTW, a little while ago while debugging some branch relaxation issues I noticed that we were overestimating the size of some instructions and thus relaxing when it wasn't needed. I made a mental note to look into that when I could. It will be useful to have these additional checks if we start tightening loose upper bounds.

[reames]

Address review comment re: non-assert builds.

[craig.topper]

llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp

293

Is any compiler smart enough to see that this is an unused method in a class in an anonymous namespace in release builds?

[reames]

ifdef helper function

[asb]

LGTM.

llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp

bool expandAtomicBinOp(MachineBasicBlock &MBB,
                          MachineBasicBlock::iterator &NextMBBI);
   bool expandAtomicMinMaxOp(MachineBasicBlock &MBB,
                             MachineBasicBlock::iterator MBBI,
                             AtomicRMWInst::BinOp, bool IsMasked, int Width,
                             MachineBasicBlock::iterator &NextMBBI);
   bool expandAtomicCmpXchg(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator MBBI, bool IsMasked,
                            int Width, MachineBasicBlock::iterator &NextMBBI);
+#ifndef NDEBUG
+  unsigned getInstSizeInBytes(const MachineFunction &MF) const {
+    unsigned Size = 0;
+    for (auto &MBB : MF)
+      for (auto &MI : MBB)
+        Size += TII->getInstSizeInBytes(MI);
+    return Size;
+  }
+#endif
 };
 
 char RISCVExpandAtomicPseudo::ID = 0;
 
 bool RISCVExpandAtomicPseudo::runOnMachineFunction(MachineFunction &MF) {
   TII = static_cast<const RISCVInstrInfo *>(MF.getSubtarget().getInstrInfo());
+
+#ifndef NDEBUG
+  const unsigned OldSize = getInstSizeInBytes(MF);
+#endif
+
   bool Modified = false;
   for (auto &MBB : MF)
     Modified |= expandMBB(MBB);
+
+#ifndef NDEBUG
+  const unsigned NewSize = getInstSizeInBytes(MF);
+  assert(OldSize >= NewSize);
+#endif
   return Modified;
 }
 
 bool RISCVExpandAtomicPseudo::expandMBB(MachineBasicBlock &MBB) {
   bool Modified = false;
 
   MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
   while (MBBI != E) {
     MachineBasicBlock::iterator NMBBI = std::next(MBBI);

llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp

private:
   bool expandMBB(MachineBasicBlock &MBB);
   bool expandMI(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                 MachineBasicBlock::iterator &NextMBBI);
   bool expandCCOp(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI,
                   MachineBasicBlock::iterator &NextMBBI);
   bool expandVSetVL(MachineBasicBlock &MBB, MachineBasicBlock::iterator MBBI);
   bool expandVMSET_VMCLR(MachineBasicBlock &MBB,
                          MachineBasicBlock::iterator MBBI, unsigned Opcode);
+#ifndef NDEBUG
+  unsigned getInstSizeInBytes(const MachineFunction &MF) const {
+    unsigned Size = 0;
+    for (auto &MBB : MF)
+      for (auto &MI : MBB)
+        Size += TII->getInstSizeInBytes(MI);
+    return Size;
+  }
+#endif
 };
 
 char RISCVExpandPseudo::ID = 0;
 
 bool RISCVExpandPseudo::runOnMachineFunction(MachineFunction &MF) {
   TII = static_cast<const RISCVInstrInfo *>(MF.getSubtarget().getInstrInfo());
+
+#ifndef NDEBUG
+  const unsigned OldSize = getInstSizeInBytes(MF);
+#endif
+
   bool Modified = false;
   for (auto &MBB : MF)
     Modified |= expandMBB(MBB);
+
+#ifndef NDEBUG
+  const unsigned NewSize = getInstSizeInBytes(MF);
+  assert(OldSize >= NewSize);
+#endif
   return Modified;
 }
 
 bool RISCVExpandPseudo::expandMBB(MachineBasicBlock &MBB) {
   bool Modified = false;
 
   MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
   while (MBBI != E) {
bool expandLoadAddress(MachineBasicBlock &MBB,
                          MachineBasicBlock::iterator MBBI,
                          MachineBasicBlock::iterator &NextMBBI);
   bool expandLoadTLSIEAddress(MachineBasicBlock &MBB,
                               MachineBasicBlock::iterator MBBI,
                               MachineBasicBlock::iterator &NextMBBI);
   bool expandLoadTLSGDAddress(MachineBasicBlock &MBB,
                               MachineBasicBlock::iterator MBBI,
                               MachineBasicBlock::iterator &NextMBBI);
+#ifndef NDEBUG
+  unsigned getInstSizeInBytes(const MachineFunction &MF) const {
+    unsigned Size = 0;
+    for (auto &MBB : MF)
+      for (auto &MI : MBB)
+        Size += TII->getInstSizeInBytes(MI);
+    return Size;
+  }
+#endif
 };
 
 char RISCVPreRAExpandPseudo::ID = 0;
 
 bool RISCVPreRAExpandPseudo::runOnMachineFunction(MachineFunction &MF) {
   TII = static_cast<const RISCVInstrInfo *>(MF.getSubtarget().getInstrInfo());
+
+#ifndef NDEBUG
+  const unsigned OldSize = getInstSizeInBytes(MF);
+#endif
+
   bool Modified = false;
   for (auto &MBB : MF)
     Modified |= expandMBB(MBB);
+
+#ifndef NDEBUG
+  const unsigned NewSize = getInstSizeInBytes(MF);
+  assert(OldSize >= NewSize);
+#endif
   return Modified;
 }
 
 bool RISCVPreRAExpandPseudo::expandMBB(MachineBasicBlock &MBB) {
   bool Modified = false;
 
   MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
   while (MBBI != E) {