[MachineInstr] return mayAlias for not mayLoadOrStore instructions.

[shchenz]

Summary

This is a typo fix for bool MachineInstr::mayAlias() function.

For now mayAlias(CallInstruction, LoadInstruction) always returns false (NoAlias), this is not right. CallInstruction may alter the memory where the LoadInstruction operates on.

[efriedma]

Hmm.

Maybe we should be explicitly checking for isCall here? It's not really right to assume instructions that any instruction that doesn't have mayLoad/mayStore modifies memory.

[shchenz]

explicitly check for isCall

[shchenz]

Hi @efriedma could you please help to have another look for this simple fix? Thanks.

[shchenz]

gentle ping

[shchenz]

This is a quite 'small' patch and it should be a NFC patch as if any existing case hits this pattern, it gets a wrong aliasing query result.

I will commit this first and welcome your post-commit comments.

[lebedev.ri]

For starters, this is missing a test.

[shchenz]

In D87490#2316034, @lebedev.ri wrote:

For starters, this is missing a test.

@lebedev.ri thanks for looking into this, Roman. This patch protects protential alias query between a call instruction and a call/load/store instruction. Currently there is no such query in clang/llvm code base. So if we don't add a problematic alias query first in clang/llvm code base, there will be no difference with/without this patch. So I leave this without a test. Hope this is ok.

[lebedev.ri]

In D87490#2316705, @shchenz wrote:

In D87490#2316034, @lebedev.ri wrote:

For starters, this is missing a test.

@lebedev.ri thanks for looking into this, Roman. This patch protects protential alias query between a call instruction and a call/load/store instruction. Currently there is no such query in clang/llvm code base. So if we don't add a problematic alias query first in clang/llvm code base, there will be no difference with/without this patch. So I leave this without a test. Hope this is ok.

So why are we adding dead code then?

[shchenz]

So why are we adding dead code then?

I think this is necessary for the mayAlias interface. Alias query always contains the query for a call instruction. If we call this interface based on MIR for a call instruction, we get a wrong result. This was from my development, I originally thought this function can handle call instruciton, but as a result it can not. so I called this interface and my codes made some loads sink down crossing a call instruction. This was not right. The call alters the memory where the loads got content.

So I think we need to add more conditions in mayAlias interface.

[lebedev.ri]

I think we are talking past each other.
Either the code you've added can be triggered, and therefore needs a test,
or it can't be triggered/reached, and it should be removed as dead code.

[shchenz]

I think to make a fundamental interface strong, it is always possible to add some unreachable path for current code base to protect some furture improper callers. This is very like the assert statement. There should always no code path can trigger the assertion.

If you insist, I can revert this or I can change it to a assertion.

llvm/lib/CodeGen/MachineInstr.cpp

 }
 
 bool MachineInstr::mayAlias(AAResults *AA, const MachineInstr &Other,
                             bool UseTBAA) const {
   const MachineFunction *MF = getMF();
   const TargetInstrInfo *TII = MF->getSubtarget().getInstrInfo();
   const MachineFrameInfo &MFI = MF->getFrameInfo();
 
+  // Execulde call instruction which may alter the memory but can not be handled
+  // by this function.
+  if (isCall() || Other.isCall())
+    return true;
+
   // If neither instruction stores to memory, they can't alias in any
   // meaningful way, even if they read from the same address.
   if (!mayStore() && !Other.mayStore())
     return false;
 
   // Both instructions must be memory operations to be able to alias.
   if (!mayLoadOrStore() || !Other.mayLoadOrStore())
     return false;