This is an archive of the discontinued LLVM Phabricator instance.

[ARM][MVE] Tail-predication: remove the BTC + 1 overflow checks
ClosedPublic

Authored by SjoerdMeijer on Aug 20 2020, 9:33 AM.

Details

Summary

Adapt tail-predication to the new semantics of get.active.lane.mask as proposed in D86147. This means that:

  • we can remove BTC + 1 overflow checks because now the loop tripcount is passed in to the intrinsic,
  • we can immediately use that value to setup a counter for the number of elements processed by the loop and don't need to materialize BTC + 1.

Diff Detail

Event Timeline

SjoerdMeijer created this revision.Aug 20 2020, 9:33 AM
SjoerdMeijer requested review of this revision.Aug 20 2020, 9:33 AM

Now with all tests fixed.

This looks much more simple :)

llvm/lib/Target/ARM/MVETailPredication.cpp
377

nit: maybe something more descriptive that highlights that this is the original scalar trip count or the number of elements.

394

same kind of nit, I guess having scalar and vector trip count names would be most clear?

399

Could you add a comment on why we're using SignedRange here? And below for the ranges and ceiling.

SjoerdMeijer added inline comments.Aug 24 2020, 6:30 AM
llvm/lib/Target/ARM/MVETailPredication.cpp
394

Yeah, it was a bit of an overloaded tripcount mess. I will update this and use these 2 terms:

  • element count: the original scalar tripcount,
  • tripcount = the (vector) tripcount.
SjoerdMeijer added inline comments.Aug 24 2020, 7:04 AM
llvm/lib/Target/ARM/MVETailPredication.cpp
399

Hm, good catch I think. The ranges are signed because they can just be negative, but extracting the ZExt value here for that probably doesn't make any sense. I will put a fixme in for that now, because after addressing the TODO above, the:

// 1) TODO: Check that the TripCount (TC) belongs ...

which is addressed in D86074, revising this part here is highest on my list.

SjoerdMeijer edited the summary of this revision. (Show Details)

Addressed comments

This revision is now accepted and ready to land.Aug 25 2020, 12:02 AM