Page MenuHomePhabricator

[ARM][LowOverheadLoops] Fix branch target codegen
ClosedPublic

Authored by samparker on Jul 12 2019, 12:09 AM.

Details

Summary

While lowering test.set.loop.iterations, it wasn't checked how the brcond was using the result and so the wls could branch to the loop preheader instead of not entering it. The same was true for loop.decrement.reg.
So brcond and br_cc and now lowered manually when using the hwloop intrinsics. During this we now check whether the result has been negated and whether we're using SETEQ or SETNE and 0 or 1. We can then figure out which basic block the WLS and LE should be targeting.

Diff Detail

Event Timeline

samparker created this revision.Jul 12 2019, 12:09 AM
SjoerdMeijer added inline comments.Jul 12 2019, 12:52 AM
lib/Target/ARM/ARMISelLowering.cpp
12997

nit: IsLoopIntrinsic is suggesting it is returning a bool, perhaps searchLoopIntrinsic

13038

nit: interested -> interested in?

13045

nit: "... check that how ..." -> "check how"?

13076

Silly question: can you remind me why we are only looking at EQ and NE?
In tests only these 2 conditions are used. Does it make sense to test others as well, see what happens?

lib/Target/ARM/ARMISelLowering.h
129

nit: all capitals for consistency?

samparker marked an inline comment as done.Jul 12 2019, 1:02 AM
samparker added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
13076

Yeah, for loop decrement that could happen... I'll add some more tests.

samparker updated this revision to Diff 211045.Jul 22 2019, 4:01 AM
  • renamed the nodes and the search function.
  • added support for other setcc opcodes, other than EQ and NE.
samparker updated this revision to Diff 211093.Jul 22 2019, 7:11 AM

Added support for setcc ge/uge, 1

SjoerdMeijer accepted this revision.Jul 23 2019, 6:27 AM

looks good to me to, just some nits inline.

lib/Target/ARM/ARMISelLowering.cpp
13105

perhaps an assert we're expecting a BR_CC here?

13139

I was wondering if we can return here; not sure though if that is better/more robust.

13172

but in that case we don't need this unreachable.

This revision is now accepted and ready to land.Jul 23 2019, 6:27 AM
samparker marked 3 inline comments as done.Jul 23 2019, 6:42 AM
samparker added inline comments.
lib/Target/ARM/ARMISelLowering.cpp
13105

sure.

13139

At this point we know we have a loop intrinsic that we need to handle it.

13172

good point.

SjoerdMeijer added inline comments.Jul 23 2019, 7:08 AM
lib/Target/ARM/ARMISelLowering.cpp
13139

ah yes, of course, silly me!

Closed by commit rL366809: [ARM][LowOverheadLoops] Fix branch target codegen (authored by sam_parker, committed by ). · Explain WhyJul 23 2019, 7:09 AM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 7:09 AM