Page MenuHomePhabricator

Check that the key is in the LEAs map before accessing
ClosedPublic

Authored by trixirt on Aug 15 2018, 7:09 AM.

Details

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

Diff Detail

Repository
rL LLVM

Event Timeline

trixirt created this revision.Aug 15 2018, 7:09 AM

Any comments ?

kristina added subscribers: craig.topper, 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.

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 added inline comments.Aug 19 2018, 10:06 AM
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 updated this revision to Diff 161476.Aug 20 2018, 7:11 AM

Switch to using 'find' as suggested by Craig

craig.topper added inline comments.Aug 20 2018, 10:06 AM
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 updated this revision to Diff 161551.Aug 20 2018, 1:30 PM

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

This revision is now accepted and ready to land.Aug 21 2018, 10:00 AM

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

This revision was automatically updated to reflect the committed changes.