[BOLT] Provide generic implementations for isLoad/isStore

[jobnoorman]

Summary

MCInstrDesc provides the mayLoad and mayStore flags that seem
appropriate to use as a target-independent way to implement isLoad and
isStore.

I believe this is currently good enough to use for the RISC-V target as
well. I've provided a test for this that checks the generated dyno
stats (which seems to be the only thing both isLoad and isStore are
used for).

[yota9]

bolt/include/bolt/Core/MCPlusBuilder.h

617

I'm not completely sure, but shouldn't we check here that we may not store here? E.g. atomic instructions seems would return both true for store and load and the current logic is designed here to handle load-only instructions. What do you think?

[jobnoorman]

bolt/include/bolt/Core/MCPlusBuilder.h

617

Good question. As far as I can tell, this is only used for dyno stats so it should be fine to count these atomics as both loads and stores? This would already happen for the X86 target.

[yota9]

bolt/include/bolt/Core/MCPlusBuilder.h

617

Probably you're right. Also since internal implementations used inside MIB would still do what they intended to do, probably for now it is not a problem. Although I'm not completely sure do we need this extra check for the future or not :) Probably meta team would decide, but in other aspects LGTM, thanks!

[maksfb]

bolt/include/bolt/Core/MCPlusBuilder.h

617

The current implementation captures the original intent, i.e. if the instruction modifies the value in memory it should pass both isLoad() and isStore() conditions. However, when we consider predicated instructions, it is not possible to statically tell if the instruction actually loads/stores. I.e. mayLoad/mayStore is a better naming. Please feel free to change.

bolt/test/RISCV/load-store.s

4

nit:

[jobnoorman]

Use double dashes for llvm-bolt arguments

[yota9]

bolt/include/bolt/Core/MCPlusBuilder.h

617

+1 , I agree that "may" semantics looks much better in this situation :)

[jobnoorman]

bolt/include/bolt/Core/MCPlusBuilder.h

617

Agreed, committed the renaming separately in eafe4ee2e8447d7daf4aaeabcfaf8f8854dd9575.

bolt/include/bolt/Core/MCPlusBuilder.h

public:
 
   virtual void getADRReg(const MCInst &Inst, MCPhysReg &RegName) const {
     llvm_unreachable("not implemented");
   }
 
   virtual bool isMoveMem2Reg(const MCInst &Inst) const { return false; }
 
   virtual bool isLoad(const MCInst &Inst) const {
-    llvm_unreachable("not implemented");
-    return false;
+    return Info->get(Inst.getOpcode()).mayLoad();
   }
 
   virtual bool isStore(const MCInst &Inst) const {
-    llvm_unreachable("not implemented");
-    return false;
+    return Info->get(Inst.getOpcode()).mayStore();
   }
 
   virtual bool isCleanRegXOR(const MCInst &Inst) const {
     llvm_unreachable("not implemented");
     return false;
   }
 
   virtual bool isPacked(const MCInst &Inst) const {

bolt/test/RISCV/load-store.s

+// RUN: %clang %cflags -o %t %s
+// RUN: link_fdata --no-lbr %s %t %t.fdata
+// RUN: llvm-bolt %t -o /dev/null --data=%t.fdata --dyno-stats | FileCheck %s
+
+// CHECK: BOLT-INFO: program-wide dynostats after all optimizations before SCTC and FOP (no change):
+// CHECK: 3000 : executed instructions
+// CHECK: 1000 : executed load instructions
+// CHECK: 1000 : executed store instructions
+
+    .globl _start
+_start:
+# FDATA: 1 _start #_start# 1
+    ld t0, (gp)
+    sd t0, (gp)
+    ret
+    .size _start, .-_start