[NFC] Use reference instead of copies in few places

[anmyachev]

No description provided.

[ThomasRaoux]

I'm not sure those move semantic do much as the constructor currently takes arg by copy. Is there anything special you are trying to fix? I'm not sure this helps readability

[anmyachev]

constructor currently takes arg by copy

The AxisInfo constructor takes arguments by their value, yes, in this case SmallVector' copy constructor or the move constructor can be called (SmallVector has a move constructor). Since std::move casts the object to something like: SmallVector&&, the move constructor will be called, that is, we can get rid of one copy (for each SmallVector value).

I'm not sure this helps readability

It's a bit harder to read, I agree, but it saves three copies.

[lezcano]

Isn't the better fix here simply making the AxisInfo constructor take everything by const-ref? I think it's just an oversight, as the constructor doesn't use the "copy-move" idiom.

[Jokeren]

Isn't the better fix here simply making the AxisInfo constructor take everything by const-ref? I think it's just an oversight, as the constructor doesn't use the "copy-move" idiom.

I agree

[Jokeren]

Or ArrayRef<int64_t>

[anmyachev]

Done

[lezcano]

Can you take this result by reference? Is it one of those cases where the lifetime is extended?

At any rate, a better fix would be to make getOrder have NRVO semantics, but it's pretty much a microoptimisation at this point

[anmyachev]

Is it one of those cases where the lifetime is extended?

Yes, I would also like to point out that this was suggested by https://scan.coverity.com/ tool, which we use in XPU backend mostly for our files, but since some of it is duplicated, I thought it would be nice to push even such minor changes upstream to improve the code a bit.

At any rate, a better fix would be to make getOrder have NRVO semantics, but it's pretty much a microoptimisation at this point

Unfortunately I'm not sure what needs to be done for this. Can we leave it as is until this code is considered part of the hottest path?

[Jokeren]

Clean up PRs could have two distinct goals here: clarity and compilation speed. For clarity, if the current implementation is already understandable and memory-safe without significant issues, we can keep it as is. For compilation speed optimization, however, it’s unclear if altering these small areas will provide much benefit. If speeding up compilation is the priority, it would be more effective to identify which passes are time-consuming and focus optimization efforts there. Something like pass timing would be helpful https://mlir.llvm.org/docs/PassManagement/#pass-timing

[anmyachev]

Thank you very much to everyone for the comments! I will address them a bit later.

[anmyachev]

@ThomasRaoux conflicts fixed. Is there anything else I should do in this pull request?

Clean up PRs could have two distinct goals here: clarity and compilation speed. For clarity, if the current implementation is already understandable and memory-safe without significant issues, we can keep it as is. For compilation speed optimization, however, it’s unclear if altering these small areas will provide much benefit. If speeding up compilation is the priority, it would be more effective to identify which passes are time-consuming and focus optimization efforts there. Something like pass timing would be helpful https://mlir.llvm.org/docs/PassManagement/#pass-timing

@Jokeren my goal is much simpler - to port small changes that the static analyzer finds. However, thanks for the advice, now I know where to look when I need to specifically optimize passes!