Page MenuHomePhabricator

[ARM] Improve WLS lowering
ClosedPublic

Authored by dmgreen on Mar 1 2021, 1:40 PM.

Details

Summary

Recently we improved the lowering of low overhead loops and tail predicated loops, but concentrated first on the DLS do style loops. This extends those improvements over to the WLS while loops, improving the chance of lowering them successfully. To do this the lowering has to change a little as the instructions are terminators that produce a value - something that needs to be treated carefully.

Lowering starts at the Hardware Loop pass, inserting a new llvm.test.start.loop.iterations that produces both an i1 to control the loop entry and an i32 similar to the llvm.start.loop.iterations intrinsic added for do loops. This feeds into the loop phi, properly gluing the values together:

  %wls = call { i32, i1 } @llvm.test.start.loop.iterations.i32(i32 %div)
  %wls0 = extractvalue { i32, i1 } %wls, 0
  %wls1 = extractvalue { i32, i1 } %wls, 1
  br i1 %wls1, label %loop.ph, label %loop.exit
...
loop:
  %lsr.iv = phi i32 [ %wls0, %loop.ph ], [ %iv.next, %loop ]
  ..
  %iv.next = call i32 @llvm.loop.decrement.reg.i32(i32 %lsr.iv, i32 1)
  %cmp = icmp ne i32 %iv.next, 0
  br i1 %cmp, label %loop, label %loop.exit

The llvm.test.start.loop.iterations need to be lowered through ISel lowering as a pair of WLS and WLSSETUP nodes, which each get converted to t2WhileLoopSetup and t2WhileLoopStart Pseudos. This helps prevent t2WhileLoopStart from being a terminator that produces a value, something difficult to control at that stage in the pipeline. Instead the t2WhileLoopSetup produces the value of LR (essentially acting as a lr = subs rn, 0), t2WhileLoopStart consumes that lr value (the Bcc).

These are then converted into a single t2WhileLoopStartLR at the same point as t2DoLoopStartTP and t2LoopEndDec. Otherwise we revert the loop to prevent them from progressing further in the pipeline. The t2WhileLoopStartLR is the a single instruction that takes a GPR and produces LR, similar to the WLS instruction.

  %1:gprlr = t2WhileLoopStartLR %0:rgpr, %bb.3
  t2B %bb.1
...
bb.2.loop:
  %2:gprlr = PHI %1:gprlr, %bb.1, %3:gprlr, %bb.2
  ...
  %3:gprlr = t2LoopEndDec %2:gprlr, %bb.2
  t2B %bb.3

The t2WhileLoopStartLR can then be treated similar to the other low overhead loop pseudos, eventually being lowered to a WLS providing the branches are within range.

Diff Detail

Event Timeline

dmgreen created this revision.Mar 1 2021, 1:40 PM
dmgreen requested review of this revision.Mar 1 2021, 1:40 PM
Herald added a project: Restricted Project. · View Herald TranscriptMar 1 2021, 1:40 PM
SjoerdMeijer added inline comments.Mar 2 2021, 3:18 AM
llvm/docs/LangRef.rst
15969

Let's separate the LangRef part out and let's make this patch depends on that.

I think the spec change can be a bit more precise:

  • *integer* values are expected and produced,
  • are there no surprises in behaviour for some corner case values, like the value 0?
  • would be good to say what the values actually represents.

Following from this, but correct me if I am wrong, I believe there are a few things we could check: we are expecting the same integer types, which I think could be verifier checks, and we could even check the same (constant) values. I am not entirely sure if that belongs in Verifier.cpp or Lint.cpp, but somewhere there I guess?

dmgreen updated this revision to Diff 328115.Mar 4 2021, 4:04 AM

Thanks for taking a look. I'm not sure this is best to split out into a separate review though. Just as in D89881, it makes sense to keep the langref and the codegen changes together, as an atomic commit. Because the intinsics is specified as:

def int_test_start_loop_iterations :
  DefaultAttrsIntrinsic<[llvm_anyint_ty, llvm_i1_ty], [LLVMMatchType<0>], [IntrNoDuplicate]>;

.. It should be correct by construction, with the types matching between into and output. I don't think there's much to verify really, as these intrinsics are only generated by the hardware loop pass and consumed by ISel. As the reference says,

These intrinsics may be modified in the future and are not intended to be used
outside the backend. Thus, front-end and mid-level optimizations should not be
generating these intrinsics.

I've tried to add some extra details, like the other intrinsics.

SjoerdMeijer added inline comments.Mar 10 2021, 5:51 AM
llvm/docs/LangRef.rst
15985

Sorry a bit of names bike shedding. But the context of this is that I found the code/diff difficult to read because these intrinsic names look so similar and found it difficult to remember their exact differences. So I was wondering if the 'start' in the name here could e.g. be 'startup', then that would be more consistent with the instructions/pseudo this maps to?

I actually need to read everything again here, but I was also wondering if we are still using llvm.test.set.loop.iterations?

llvm/lib/Target/ARM/ARMISelLowering.h
134

I think this could benefit from some further clarifications, i.e. what the exact difference is between WLS and WLSSETUP because that may not be so obvious from these comments. I think it is well explained in the description, so something along the lines of:

Instead the t2WhileLoopSetup produces the value of LR (essentially acting as a lr = subs rn, 0), t2WhileLoopStart consumes that lr value (the Bcc).

llvm/lib/Target/ARM/ARMInstrThumb2.td
5477

Do we need to say something about isTerminator or hasSideEffects too here?

dmgreen updated this revision to Diff 329857.Mar 10 2021, 11:59 PM

Added lots of extra comments.

dmgreen added inline comments.Mar 10 2021, 11:59 PM
llvm/docs/LangRef.rst
15985

Yeah. I didn't think t2WhileLoopStartLR was particularly imaginative either.

For these hardware loop intrinsics, we either use:
llvm.set.loop.iterations and llvm.test.set.loop.iterations
or
llvm.start.loop.iterations and llvm.test.start.loop.iterations
depending on whether the backend requests UsePHICounter. We at Arm request that to make sure the values are all glued together properly, but other backends needn't. So this was attempting to be consistent with the llvm.start.loop.iterations that was changed back in D89881.

llvm/lib/Target/ARM/ARMInstrThumb2.td
5477

Do you mean as a comment? Or their attribute values? The defaults should be fine for the attributes I think. I've tried to expand the comment for all these too.

SjoerdMeijer accepted this revision.Mar 11 2021, 1:05 AM

Thanks, that definitely helped.
LGTM

llvm/lib/Target/ARM/ARMInstrThumb2.td
5468

typo: 'a'

5483

typo: psuedo

llvm/lib/Target/ARM/MVETPAndVPTOptimisationsPass.cpp
194

'Attempt' made me wonder if we need to assert if it can't be found, but am not entirely sure now of the value of such an assert.

This revision is now accepted and ready to land.Mar 11 2021, 1:05 AM
This revision was landed with ongoing or failed builds.Mar 11 2021, 9:56 AM
This revision was automatically updated to reflect the committed changes.