[CodeGen] Rename MachineInstr::isInvariantLoad to isDereferenceableInvariantLoad. NFC

[jlebar]

Summary

I want to separate out the notions of invariance and dereferenceability
at the MI level, so that they correspond to the equivalent concepts at
the IR level. (Currently an MI load is MI-invariant iff it's
IR-invariant and IR-dereferenceable.)

First step is renaming this function.

[chandlerc]

llvm/include/llvm/CodeGen/MachineInstr.h

1131–1132 ↗

(On Diff #67592)

typo: "return true of" -> "return true if"

llvm/lib/CodeGen/MachineInstr.cpp

1596 ↗

(On Diff #67592)

No need to start with the function name.

Also, I'd format the comment to make it clear what the brief summary is while you're changing it.

[jlebar]

Address comment comments.

llvm/lib/CodeGen/MachineInstr.cpp

1596 ↗

(On Diff #67592)

No need to start with the function name.

I don't like it either, but this is the prevailing style in the file, and the LLVM coding standards say (in bold) "If you are extending, enhancing, or bug fixing already implemented code, use the style that is already being used."?

Frankly if I were cleaning it up as a clean slate I'd delete the comment entirely, because it just repeats (verbatim) what's in the header.

Happy to do whatever you want here.

Also, I'd format the comment to make it clear what the brief summary is while you're changing it.

Done.

[jlebar]

Friendly ping.

[hfinkel]

llvm/include/llvm/CodeGen/MachineInstr.h

1131 ↗

(On Diff #68304)

The original text says, "invariant across the function", and it seems important that invariance here is restricted to during the execution of the current function body. Can/should you still capture this concept in the comment?

[chandlerc]

FWIW, I agree with Hal. This patch LGTM with Hal's comment addressed.

[jlebar]

The original text says, "invariant across the function", and it seems important that invariance here is restricted to during the execution of the current function body.

If the comment is correct, I agree it's important to keep that part, but I'm not sure it is correct.

The function returns true if, for each load L done by the instruction,

• L is MMO-invariant (meaning here that it is dereferenceable and has the !invariant.load IR annotation -- this patch comes before the one where we split out the notions of MMO-invariance and MMO-dereferenceability), or
• L loads from a PseudoSourceValue that's constant, or
• The address L is loading from points to constant memory.

I think this means that the load is invariant period, not invariant within the function?

[jlebar]

Reword comment.

Now that I'm looking at this again, I think we can improve the comment on this
function, but I still didn't change it back to saying that the load is
invariant across the function.

[hfinkel]

In D23370#523285, @jlebar wrote:

The original text says, "invariant across the function", and it seems important that invariance here is restricted to during the execution of the current function body.

If the comment is correct, I agree it's important to keep that part, but I'm not sure it is correct.

The function returns true if, for each load L done by the instruction,

• L is MMO-invariant (meaning here that it is dereferenceable and has the !invariant.load IR annotation -- this patch comes before the one where we split out the notions of MMO-invariance and MMO-dereferenceability), or
• L loads from a PseudoSourceValue that's constant, or

I think this is the point: These PseudoSourceValues can be things like addresses of things on the caller's stack (or on the stack in general) - they're not constant in some global sense, but they are constant during the execution of the current function. The stack itself is clearly not constant memory.

• The address L is loading from points to constant memory.

I think this means that the load is invariant period, not invariant within the function?

[reames]

llvm/include/llvm/CodeGen/MachineInstr.h

1127 ↗

(On Diff #69011)

I think this comment is missing something major. In general, dereferenceability is a context-sensative property. In this query, the dereferenceable property is not context sensative and can only return true if the location is dereferenceable for any possible context. That should be clarified.

The "any possible context" bit is the issue Hal raises about function scope vs global scope. I believe we want function scope, but that implies we have to strip the relevant flag if we ever inline at the MI level. Do we actually do that?

[jlebar]

Further update comments to clarify scope of invariance.

[jlebar]

I think this is the point: These PseudoSourceValues can be things like addresses of things on the caller's stack

Ah, right, okay, thanks for clarifying that.

In general, dereferenceability is a context-sensative property. In this query, the dereferenceable property is not context sensative and can only return true if the location is dereferenceable for any possible context. That should be clarified.

I'm happy to clarify this, but I'm not sure how. The comment talks about a load instruction never trapping, so clearly we're talking about the memory location being dereferenceable within the context of the function?

Maybe it would be helpful if you could give a specific suggestion?

The "any possible context" bit is the issue Hal raises about function scope vs global scope. I believe we want function scope, but that implies we have to strip the relevant flag if we ever inline at the MI level. Do we actually do that?

AIUI (and I've been wrong now at least once, so take it fwiw) there's nothing to strip, because if we inline at the MI level and a load that once referenced constant memory no longer references constant memory, we'll notice that and the function will dtrt.

[jlebar]

reames, Hal, friendly ping.

[hfinkel]

llvm/include/llvm/CodeGen/MachineInstr.h

1128 ↗

(On Diff #69032)

"over the course of this function" sounds a bit too colloquial to me. Can we say, "during the execution of this function."?

Regardless, this LGTM too.

llvm/trunk/include/llvm/CodeGen/MachineInstr.h

public:
   bool isSafeToMove(AliasAnalysis *AA, bool &SawStore) const;
 
   /// Return true if this instruction may have an ordered
   /// or volatile memory reference, or if the information describing the memory
   /// reference is not available. Return false if it is known to have no
   /// ordered or volatile memory references.
   bool hasOrderedMemoryRef() const;
 
-  /// Return true if this instruction is loading from a
-  /// location whose value is invariant across the function.  For example,
-  /// loading a value from the constant pool or from the argument area of
-  /// a function if it does not change.  This should only return true of *all*
-  /// loads the instruction does are invariant (if it does multiple loads).
-  bool isInvariantLoad(AliasAnalysis *AA) const;
+  /// Return true if this load instruction never traps and points to a memory
+  /// location whose value doesn't change during the execution of this function.
+  ///
+  /// Examples include loading a value from the constant pool or from the
+  /// argument area of a function (if it does not change).  If the instruction
+  /// does multiple loads, this returns true only if all of the loads are
+  /// dereferenceable and invariant.
+  bool isDereferenceableInvariantLoad(AliasAnalysis *AA) const;
 
   /// If the specified instruction is a PHI that always merges together the
   /// same virtual register, return the register, otherwise return 0.
   unsigned isConstantValuePHI() const;
 
   /// Return true if this instruction has side effects that are not modeled
   /// by mayLoad / mayStore, etc.
   /// For all instructions, the property is encoded in MCInstrDesc::Flags

llvm/trunk/lib/CodeGen/MachineCSE.cpp

bool MachineCSE::isCSECandidate(MachineInstr *MI) {
   if (MI->mayStore() || MI->isCall() || MI->isTerminator() ||
       MI->hasUnmodeledSideEffects())
     return false;
 
   if (MI->mayLoad()) {
     // Okay, this instruction does a load. As a refinement, we allow the target
     // to decide whether the loaded value is actually a constant. If so, we can
     // actually use it as a load.
-    if (!MI->isInvariantLoad(AA))
+    if (!MI->isDereferenceableInvariantLoad(AA))
       // FIXME: we should be able to hoist loads with no other side effects if
       // there are no other instructions which can change memory in this loop.
       // This is a trivial form of alias analysis.
       return false;
   }
 
   // Ignore stack guard loads, otherwise the register that holds CSEed value may
   // be spilled and get loaded back with corrupted data.

llvm/trunk/lib/CodeGen/MachineInstr.cpp

if (isPosition() || isDebugValue() || isTerminator() ||
       hasUnmodeledSideEffects())
     return false;
 
   // See if this instruction does a load.  If so, we have to guarantee that the
   // loaded value doesn't change between the load and the its intended
   // destination. The check for isInvariantLoad gives the targe the chance to
   // classify the load as always returning a constant, e.g. a constant pool
   // load.
-  if (mayLoad() && !isInvariantLoad(AA))
+  if (mayLoad() && !isDereferenceableInvariantLoad(AA))
     // Otherwise, this is a real load.  If there is a store between the load and
     // end of block, we can't move it.
     return !SawStore;
 
   return true;
 }
 
 /// hasOrderedMemoryRef - Return true if this instruction may have an ordered
if (memoperands_empty())
     return true;
 
   // Check if any of our memory operands are ordered.
   return any_of(memoperands(), [](const MachineMemOperand *MMO) {
     return !MMO->isUnordered();
   });
 }
 
-/// isInvariantLoad - Return true if this instruction is loading from a
-/// location whose value is invariant across the function.  For example,
-/// loading a value from the constant pool or from the argument area
-/// of a function if it does not change.  This should only return true of
-/// *all* loads the instruction does are invariant (if it does multiple loads).
-bool MachineInstr::isInvariantLoad(AliasAnalysis *AA) const {
+/// isDereferenceableInvariantLoad - Return true if this instruction will never
+/// trap and is loading from a location whose value is invariant across a run of
+/// this function.
+bool MachineInstr::isDereferenceableInvariantLoad(AliasAnalysis *AA) const {
   // If the instruction doesn't load at all, it isn't an invariant load.
   if (!mayLoad())
     return false;
 
   // If the instruction has lost its memoperands, conservatively assume that
   // it may not be an invariant load.
   if (memoperands_empty())
     return false;

llvm/trunk/lib/CodeGen/MachineLICM.cpp

bool MachineLICM::IsProfitableToHoist(MachineInstr &MI) {
   if (AvoidSpeculation &&
       (!IsGuaranteedToExecute(MI.getParent()) && !MayCSE(&MI))) {
     DEBUG(dbgs() << "Won't speculate: " << MI);
     return false;
   }
 
   // High register pressure situation, only hoist if the instruction is going
   // to be remat'ed.
-  if (!TII->isTriviallyReMaterializable(MI, AA) && !MI.isInvariantLoad(AA)) {
+  if (!TII->isTriviallyReMaterializable(MI, AA) &&
+      !MI.isDereferenceableInvariantLoad(AA)) {
     DEBUG(dbgs() << "Can't remat / high reg-pressure: " << MI);
     return false;
   }
 
   return true;
 }
 
 /// Unfold a load from the given machineinstr if the load itself could be
 /// hoisted. Return the unfolded and hoistable load, or null if the load
 /// couldn't be unfolded or if it wouldn't be hoistable.
 MachineInstr *MachineLICM::ExtractHoistableLoad(MachineInstr *MI) {
   // Don't unfold simple loads.
   if (MI->canFoldAsLoad())
     return nullptr;
 
   // If not, we may be able to unfold a load and hoist that.
   // First test whether the instruction is loading from an amenable
   // memory location.
-  if (!MI->isInvariantLoad(AA))
+  if (!MI->isDereferenceableInvariantLoad(AA))
     return nullptr;
 
   // Next determine the register class for a temporary register.
   unsigned LoadRegIndex;
   unsigned NewOpc =
     TII->getOpcodeAfterMemoryUnfold(MI->getOpcode(),
                                     /*UnfoldLoad=*/true,
                                     /*UnfoldStore=*/false,

llvm/trunk/lib/CodeGen/MachinePipeliner.cpp

static bool isSuccOrder(SUnit *SUa, SUnit *SUb) {
   return false;
 }
 
 /// Return true if the instruction causes a chain between memory
 /// references before and after it.
 static bool isDependenceBarrier(MachineInstr &MI, AliasAnalysis *AA) {
   return MI.isCall() || MI.hasUnmodeledSideEffects() ||
          (MI.hasOrderedMemoryRef() &&
-          (!MI.mayLoad() || !MI.isInvariantLoad(AA)));
+          (!MI.mayLoad() || !MI.isDereferenceableInvariantLoad(AA)));
 }
 
 /// Return the underlying objects for the memory references of an instruction.
 /// This function calls the code in ValueTracking, but first checks that the
 /// instruction has a memory operand.
 static void getUnderlyingObjects(MachineInstr *MI,
                                  SmallVectorImpl<Value *> &Objs,
                                  const DataLayout &DL) {

llvm/trunk/lib/CodeGen/ScheduleDAGInstrs.cpp

for (VReg2SUnit &V2SU : make_range(CurrentVRegDefs.find(Reg),
     V2SU.SU->addPred(SDep(SU, SDep::Anti, Reg));
   }
 }
 
 /// Return true if MI is an instruction we are unable to reason about
 /// (like a call or something with unmodeled side effects).
 static inline bool isGlobalMemoryObject(AliasAnalysis *AA, MachineInstr *MI) {
   return MI->isCall() || MI->hasUnmodeledSideEffects() ||
-         (MI->hasOrderedMemoryRef() && !MI->isInvariantLoad(AA));
+         (MI->hasOrderedMemoryRef() && !MI->isDereferenceableInvariantLoad(AA));
 }
 
 /// This returns true if the two MIs need a chain edge between them.
 /// This is called on normal stores and loads.
 static bool MIsNeedChainEdge(AliasAnalysis *AA, const MachineFrameInfo *MFI,
                              const DataLayout &DL, MachineInstr *MIa,
                              MachineInstr *MIb) {
   const MachineFunction *MF = MIa->getParent()->getParent();
if (isGlobalMemoryObject(AA, &MI)) {
       addBarrierChain(Loads);
       addBarrierChain(NonAliasStores);
       addBarrierChain(NonAliasLoads);
 
       continue;
     }
 
     // If it's not a store or a variant load, we're done.
-    if (!MI.mayStore() && !(MI.mayLoad() && !MI.isInvariantLoad(AA)))
+    if (!MI.mayStore() &&
+        !(MI.mayLoad() && !MI.isDereferenceableInvariantLoad(AA)))
       continue;
 
     // Always add dependecy edge to BarrierChain if present.
     if (BarrierChain)
       BarrierChain->addPredBarrier(SU);
 
     // Find the underlying objects for MI. The Objs vector is either
     // empty, or filled with the Values of memory locations which this

llvm/trunk/lib/CodeGen/TargetInstrInfo.cpp

if (MI.isNotDuplicable() || MI.mayStore() || MI.hasUnmodeledSideEffects())
     return false;
 
   // Don't remat inline asm. We have no idea how expensive it is
   // even if it's side effect free.
   if (MI.isInlineAsm())
     return false;
 
   // Avoid instructions which load from potentially varying memory.
-  if (MI.mayLoad() && !MI.isInvariantLoad(AA))
+  if (MI.mayLoad() && !MI.isDereferenceableInvariantLoad(AA))
     return false;
 
   // If any of the registers accessed are non-constant, conservatively assume
   // the instruction is not rematerializable.
   for (unsigned i = 0, e = MI.getNumOperands(); i != e; ++i) {
     const MachineOperand &MO = MI.getOperand(i);
     if (!MO.isReg()) continue;
     unsigned Reg = MO.getReg();

llvm/trunk/lib/Target/WebAssembly/WebAssemblyRegStackify.cpp

static void Query(const MachineInstr &MI, AliasAnalysis &AA, bool &Read,
                   bool &Write, bool &Effects, bool &StackPointer) {
   assert(!MI.isPosition());
   assert(!MI.isTerminator());
 
   if (MI.isDebugValue())
     return;
 
   // Check for loads.
-  if (MI.mayLoad() && !MI.isInvariantLoad(&AA))
+  if (MI.mayLoad() && !MI.isDereferenceableInvariantLoad(&AA))
     Read = true;
 
   // Check for stores.
   if (MI.mayStore()) {
     Write = true;
 
     // Check for stores to __stack_pointer.
     for (auto MMO : MI.memoperands()) {

llvm/trunk/lib/Target/X86/X86InstrInfo.cpp

bool X86InstrInfo::isReallyTriviallyReMaterializable(const MachineInstr &MI,
   case X86::VMOVUPSZ128rm:
   case X86::VMOVUPSZ256rm:
   case X86::VMOVUPSZrm: {
     // Loads from constant pools are trivially rematerializable.
     if (MI.getOperand(1 + X86::AddrBaseReg).isReg() &&
         MI.getOperand(1 + X86::AddrScaleAmt).isImm() &&
         MI.getOperand(1 + X86::AddrIndexReg).isReg() &&
         MI.getOperand(1 + X86::AddrIndexReg).getReg() == 0 &&
-        MI.isInvariantLoad(AA)) {
+        MI.isDereferenceableInvariantLoad(AA)) {
       unsigned BaseReg = MI.getOperand(1 + X86::AddrBaseReg).getReg();
       if (BaseReg == 0 || BaseReg == X86::RIP)
         return true;
       // Allow re-materialization of PIC load.
       if (!ReMatPICStubLoad && MI.getOperand(1 + X86::AddrDisp).isGlobal())
         return false;
       const MachineFunction &MF = *MI.getParent()->getParent();
       const MachineRegisterInfo &MRI = MF.getRegInfo();