This is an archive of the discontinued LLVM Phabricator instance.

[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

Repository
rL LLVM

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 ↗(On Diff #209418)

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

13038 ↗(On Diff #209418)

nit: interested -> interested in?

13045 ↗(On Diff #209418)

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

13076 ↗(On Diff #209418)

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 ↗(On Diff #209418)

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 ↗(On Diff #209418)

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 ↗(On Diff #211093)

perhaps an assert we're expecting a BR_CC here?

13139 ↗(On Diff #211093)

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

13172 ↗(On Diff #211093)

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 ↗(On Diff #211093)

sure.

13139 ↗(On Diff #211093)

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

13172 ↗(On Diff #211093)

good point.

SjoerdMeijer added inline comments.Jul 23 2019, 7:08 AM
lib/Target/ARM/ARMISelLowering.cpp
13139 ↗(On Diff #211093)

ah yes, of course, silly me!

This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJul 23 2019, 7:09 AM