Page MenuHomePhabricator

[ARM] Fix perf regression in compare optimization.
ClosedPublic

Authored by jgalenson on Jan 18 2018, 1:10 PM.

Details

Summary

Fix a performance regression caused by r322737.

While trying to make it easier to replace compares with existing adds and subtracts, I accidentally stopped it from doing so in some cases. This should fix that. I'm also fixing another potential bug in that commit.

Diff Detail

Event Timeline

jgalenson created this revision.Jan 18 2018, 1:10 PM

The problem was the "--E" in my previous commit (about 25 lines above the changed line). That caused the loop just below it (containing this line) to run in some cases it did not before. If I is the first instruction in the block, it now enters the loop and then returns false at the line I changed. I modified that to break out of the loop instead, which allows us to still consider MI as a candidate.

fhahn added a subscriber: fhahn.Jan 18 2018, 1:15 PM
efriedma added inline comments.Jan 18 2018, 3:28 PM
lib/Target/ARM/ARMBaseInstrInfo.cpp
2743

Is there some reason to expect that --E is a valid, given E could be the first instruction in its basic block?

2750

This loop is confusing. The initial value of I is the compare. You're looping until it equals E... but you also have an early exit if you hit the beginning of the basic block.

Could you fix the loop to explicitly compute its exit condition in some more intuitive way? Maybe you could express this more clearly using MachineBasicBlock::reverse_iterator. And ideally you could use range-based for loop with "make_range" rather an explicit increment/decrement.

jgalenson added inline comments.Jan 18 2018, 4:23 PM
lib/Target/ARM/ARMBaseInstrInfo.cpp
2743

Good catch. I'll guard this.

2750

I agree that the loop is confusing, but I'd rather make that change as part of a separate commit, since doing so is surprisingly non-trivial and could well introduce new bugs.

jgalenson updated this revision to Diff 130518.Jan 18 2018, 4:25 PM
jgalenson edited the summary of this revision. (Show Details)

This guards the potential bug Eli pointed out.

This isn't a perfect solution, since if E is the first statement in the block, the loop won't search it (and the loop structure is confusing). Hopefully we can fix that in a follow-up commit.

efriedma accepted this revision.Jan 18 2018, 4:33 PM

LGTM with the comment fixed.

lib/Target/ARM/ARMBaseInstrInfo.cpp
2750

Okay.

2765–2766

Please fix this comment; there is no 'and', and instructions after the comparison are irrelevant.

This revision is now accepted and ready to land.Jan 18 2018, 4:33 PM

Thanks.

lib/Target/ARM/ARMBaseInstrInfo.cpp
2765–2766

For what it's worth, I believe the 'and' refers to the block at the beginning of the function, which can set MI to be equal to an 'and'. But I agree that this comment is confusing, so I'll remove it.

Hi Joel,

I have benchmarked the patch. It fixes the regressions.

Thanks,
Evgeny Astigeevich

Thanks, Evgeny! I'll fix the comment and commit it now and then continue to work on cleaning up this loop.

jgalenson updated this revision to Diff 130637.Jan 19 2018, 9:35 AM

Thanks, Evgeny! I'll fix the comment and commit it now and then continue to work on cleaning up this loop.

Let me know if you need some benchmarking.

Thanks,
Evgeny

This revision was automatically updated to reflect the committed changes.