[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

81

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

81

Warn for unused variables I mean.

[reames]

llvm/lib/Target/RISCV/RISCVExpandAtomicPseudoInsts.cpp

81

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

286

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 expandAtomicCmpXchg(MachineBasicBlock &MBB,
                            MachineBasicBlock::iterator MBBI, bool IsMasked,
                            int Width, MachineBasicBlock::iterator &NextMBBI);
 };
 
 char RISCVExpandAtomicPseudo::ID = 0;
 
 bool RISCVExpandAtomicPseudo::runOnMachineFunction(MachineFunction &MF) {
   TII = static_cast<const RISCVInstrInfo *>(MF.getSubtarget().getInstrInfo());
+
+  unsigned OldSize = 0;
+  for (auto &MBB : MF)
+    for (auto &MI : MBB)
+      OldSize += TII->getInstSizeInBytes(MI);
+
   bool Modified = false;
   for (auto &MBB : MF)
     Modified |= expandMBB(MBB);
+
+  unsigned NewSize = 0;
+  for (auto &MBB : MF)
+    for (auto &MI : MBB)
+      NewSize += TII->getInstSizeInBytes(MI);
+  assert(OldSize >= NewSize);
   return Modified;
 }
 
 bool RISCVExpandAtomicPseudo::expandMBB(MachineBasicBlock &MBB) {
   bool Modified = false;
 
   MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
   while (MBBI != E) {

llvm/lib/Target/RISCV/RISCVExpandPseudoInsts.cpp

private:
   bool expandVMSET_VMCLR(MachineBasicBlock &MBB,
                          MachineBasicBlock::iterator MBBI, unsigned Opcode);
 };
 
 char RISCVExpandPseudo::ID = 0;
 
 bool RISCVExpandPseudo::runOnMachineFunction(MachineFunction &MF) {
   TII = static_cast<const RISCVInstrInfo *>(MF.getSubtarget().getInstrInfo());
+
+  unsigned OldSize = 0;
+  for (auto &MBB : MF)
+    for (auto &MI : MBB)
+      OldSize += TII->getInstSizeInBytes(MI);
+
   bool Modified = false;
   for (auto &MBB : MF)
     Modified |= expandMBB(MBB);
+
+  unsigned NewSize = 0;
+  for (auto &MBB : MF)
+    for (auto &MI : MBB)
+      NewSize += TII->getInstSizeInBytes(MI);
+  assert(OldSize >= NewSize);
   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);
 };
 
 char RISCVPreRAExpandPseudo::ID = 0;
 
 bool RISCVPreRAExpandPseudo::runOnMachineFunction(MachineFunction &MF) {
   TII = static_cast<const RISCVInstrInfo *>(MF.getSubtarget().getInstrInfo());
+
+  unsigned OldSize = 0;
+  for (auto &MBB : MF)
+    for (auto &MI : MBB)
+      OldSize += TII->getInstSizeInBytes(MI);
+
   bool Modified = false;
   for (auto &MBB : MF)
     Modified |= expandMBB(MBB);
+
+  unsigned NewSize = 0;
+  for (auto &MBB : MF)
+    for (auto &MI : MBB)
+      NewSize += TII->getInstSizeInBytes(MI);
+  assert(OldSize >= NewSize);
   return Modified;
 }
 
 bool RISCVPreRAExpandPseudo::expandMBB(MachineBasicBlock &MBB) {
   bool Modified = false;
 
   MachineBasicBlock::iterator MBBI = MBB.begin(), E = MBB.end();
   while (MBBI != E) {