Page MenuHomePhabricator

[IR] Split vscale_range interface
ClosedPublic

Authored by c-rhodes on Nov 17 2021, 3:42 AM.

Details

Summary

Interface is split from:

std::pair<unsigned, unsigned> getVScaleRangeArgs()

into separate functions for min/max:

unsigned getVScaleRangeMin();
Optional<unsigned> getVScaleRangeMax();

Diff Detail

Event Timeline

c-rhodes created this revision.Nov 17 2021, 3:42 AM
c-rhodes requested review of this revision.Nov 17 2021, 3:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptNov 17 2021, 3:42 AM
bsmith added inline comments.Nov 18 2021, 7:20 AM
llvm/include/llvm/IR/Attributes.h
223

I'm not sure how I feel about this being an Optional type, all of the code changes below are still having to check max against 0, it seems that having this be Optional is just complicating the code for no gain.

Either this should not be Optional, or a value of 0 should be disallowed for max from this interface.

paulwalker-arm added inline comments.Nov 18 2021, 7:24 AM
llvm/include/llvm/IR/Attributes.h
223

I agree. I believe the intent is for getVScaleRangeMax to only return a real >0 value or None otherwise.

c-rhodes updated this revision to Diff 391610.Dec 3 2021, 4:33 AM

return None for unbounded in getVScaleRangeMax

c-rhodes added inline comments.Dec 3 2021, 4:34 AM
llvm/include/llvm/IR/Attributes.h
223

thanks for comments and sorry for only just getting back to you, I've updated the patch so 0 is replaced with None in the interface, hopefully this makes more sense now.

sdesmalen added inline comments.Dec 3 2021, 4:59 AM
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
1609–1610

nit: this is probably a stylistic thing, but I personally find the following a bit nicer to read:

Attribute Attr = CI.getFunction()->getFnAttribute(Attribute::VScaleRange);
if (Optional<unsigned> MaxVScale = Attr.getVScaleRangeMax())
  if (Log2_32(MaxVScale.getValue()) < (SrcBitSize - 1)) {
    ..
  }

(same suggestion for the similar cases)

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5368–5371

nit: similar suggestion here:

Attribute Attr = CI.getFunction()->getFnAttribute(Attribute::VScaleRange);
if (Optional<unsigned> Max = Attr.getVScaleRangeMax())
  MaxVScale = Max.getValue();
c-rhodes updated this revision to Diff 391615.Dec 3 2021, 5:24 AM

Address comments

c-rhodes marked an inline comment as done.Dec 3 2021, 5:24 AM
c-rhodes added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineCasts.cpp
1609–1610

nit: this is probably a stylistic thing, but I personally find the following a bit nicer to read:

Attribute Attr = CI.getFunction()->getFnAttribute(Attribute::VScaleRange);
if (Optional<unsigned> MaxVScale = Attr.getVScaleRangeMax())
  if (Log2_32(MaxVScale.getValue()) < (SrcBitSize - 1)) {
    ..
  }

(same suggestion for the similar cases)

I agree, done

Thanks, I left two more minor comments that I didn't spot earlier. Otherwise looks good to me.

llvm/include/llvm/IR/Attributes.h
222–223

s/. If omitted, this will be the minimum,//

This probably meant to say "if omitted in the textual representation of the LLVM IR attribute", but I'm not sure how that's relevant for this comment.

1063–1064

Same here, please just omit that part of the comment.

c-rhodes updated this revision to Diff 391618.Dec 3 2021, 5:41 AM

Update interface comment

c-rhodes marked 2 inline comments as done.Dec 3 2021, 5:42 AM
sdesmalen accepted this revision.Dec 3 2021, 5:43 AM

Thanks @c-rhodes, LGTM

This revision is now accepted and ready to land.Dec 3 2021, 5:43 AM
paulwalker-arm added inline comments.Dec 3 2021, 9:03 AM
llvm/include/llvm/IR/Attributes.h
222–223

Up to you but I think "or None when unknown." is simpler.

1063

As above.

llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5369–5370

Is this if statement required? MaxVScale is an optional so I think MaxVScale = Attr.getVScaleRangeMax(); just works?

c-rhodes updated this revision to Diff 391662.Dec 3 2021, 9:34 AM

Address comments

c-rhodes marked 3 inline comments as done.Dec 3 2021, 9:36 AM
c-rhodes added inline comments.
llvm/lib/Transforms/Vectorize/LoopVectorize.cpp
5369–5370

Is this if statement required? MaxVScale is an optional so I think MaxVScale = Attr.getVScaleRangeMax(); just works?

I wondered that, you're right it works.

paulwalker-arm accepted this revision.Dec 3 2021, 9:39 AM
This revision was landed with ongoing or failed builds.Dec 7 2021, 2:43 AM
This revision was automatically updated to reflect the committed changes.
c-rhodes marked an inline comment as done.