Check that the key is in the LEAs map before accessing

[trixirt]

Summary

When the key is not already in the map, the access operator[] creates an empty value and grows the map.
Resizing a map is very slow, so this needs to be avoided.

Found with csmith + asserts.

May help with
https://bugs.llvm.org/show_bug.cgi?id=25843

[trixirt]

Any comments ?

[kristina]

This looks like intentional behavior, added @craig.topper to review.

I think you're changing the semantics of the code by removing a side effect, may lead to unexpected bugs somewhere else.

[trixirt]

I don't think there are any meaningful side effects.

In this function, removeRedundantAddrCalc, when genMemOpKey returns a key not already in the map, the value/vector on instructions is empty.
When chooseBestLEA is passed an empty vector, it never loops over it an returns a nullptr.
A nullptr causes the mi loop to continue

If on a future mi loop pass, there was a match, the match would return the same empty value/vector
and cause the same continue in the mi loop

When removeRedundantAddrCalc returns, it is only to 1 place in the main runOnMachineFunction work loop.
The LEAs map's scope is the work loop and removeRedundantAddrCalc is it's last use.

[craig.topper]

lib/Target/X86/X86OptimizeLEAs.cpp

519 ↗

(On Diff #160794)

Can we use LEAs.find so we can use one map search for the existence check and getting the SmallVectorImpl for chooseBestLEA?

[trixirt]

Switch to using 'find' as suggested by Craig

[craig.topper]

lib/Target/X86/X86OptimizeLEAs.cpp

517 ↗

(On Diff #161476)

Can you just call getMemOpKey when you call find? I don't think we need a separate variable for it.

518 ↗

(On Diff #161476)

Variable names should be capitalized.

[trixirt]

Changes suggested by Craig.
For readability, move chooseBestLEA's variables closer to call.

[craig.topper]

LGTM

[trixirt]

I do not have commit perms, so can someone commit this for me ?

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

for (auto I = MBB->begin(), E = MBB->end(); I != E;) {
     int MemOpNo = X86II::getMemoryOperandNo(Desc.TSFlags);
 
     // If instruction has no memory operand - skip it.
     if (MemOpNo < 0)
       continue;
 
     MemOpNo += X86II::getOperandBias(Desc);
 
+    // Do not call chooseBestLEA if there was no matching LEA
+    auto Insns = LEAs.find(getMemOpKey(MI, MemOpNo));
+    if (Insns == LEAs.end())
+      continue;
+
     // Get the best LEA instruction to replace address calculation.
     MachineInstr *DefMI;
     int64_t AddrDispShift;
     int Dist;
-    if (!chooseBestLEA(LEAs[getMemOpKey(MI, MemOpNo)], MI, DefMI, AddrDispShift,
-                       Dist))
+    if (!chooseBestLEA(Insns->second, MI, DefMI, AddrDispShift, Dist))
       continue;
 
     // If LEA occurs before current instruction, we can freely replace
     // the instruction. If LEA occurs after, we can lift LEA above the
     // instruction and this way to be able to replace it. Since LEA and the
     // instruction have similar memory operands (thus, the same def
     // instructions for these operands), we can always do that, without
     // worries of using registers before their defs.