AMDGPU/SILoadStoreOptimizer: fix a likely bug introduced recently

[nhaehnle]

Summary

We should check for same instruction class before checking whether they
have the same base address, else we might iterate out of bounds of a
MachineInstr operands list. The InstClass check is also cheaper.

This was introduced in SVN r373630.

[arsenm]

Testcase?

[nhaehnle]

In D68690#1701685, @arsenm wrote:

Testcase?

I didn't really have the bandwidth to think deeply about how to trigger this. It's more of a drive-by observation.

[nhaehnle]

Ping

On the issue of a test case: I don't think it's practically possible to hit the crash I was worried about right now, because AddrIdx[0] happens to be small enough in all cases that it will be a valid operand index in all cases, and then hasSameBaseAddress will bail out for machine instructions of a different class because the register comparison of the first index will fail. However, that's just an artifact of how machine operand indices happen to be laid out today. I still maintain that the change itself is the right way to do things.

[nhaehnle]

ping^2

[arsenm]

LGTM

llvm/lib/Target/AMDGPU/SILoadStoreOptimizer.cpp

bool SILoadStoreOptimizer::promoteConstantOffsetToImm(
   }
 
   return false;
 }
 
 void SILoadStoreOptimizer::addInstToMergeableList(const CombineInfo &CI,
                  std::list<std::list<CombineInfo> > &MergeableInsts) const {
   for (std::list<CombineInfo> &AddrList : MergeableInsts) {
-    if (AddrList.front().hasSameBaseAddress(*CI.I) &&
-        AddrList.front().InstClass == CI.InstClass) {
+    if (AddrList.front().InstClass == CI.InstClass &&
+        AddrList.front().hasSameBaseAddress(*CI.I)) {
       AddrList.emplace_back(CI);
       return;
     }
   }
 
   // Base address not found, so add a new list.
   MergeableInsts.emplace_back(1, CI);
 }