This is an archive of the discontinued LLVM Phabricator instance.

Store (cache) the Argument number (index in the argument list) inside the BlockArgumentImpl (NFC)
ClosedPublic

Authored by mehdi_amini on Feb 26 2021, 4:13 PM.

Details

Summary

This avoids linear search in BlockArgument::getArgNumber().

Diff Detail

Event Timeline

mehdi_amini created this revision.Feb 26 2021, 4:13 PM
mehdi_amini requested review of this revision.Feb 26 2021, 4:13 PM
rriddle added inline comments.Feb 26 2021, 4:20 PM
mlir/include/mlir/IR/Value.h
287

The comment here is off.

288

Can you make this private? Block is already a friend.

mlir/lib/IR/Block.cpp
161

+ index + 1? The argument at index should already have its index set. Also, llvm::drop_begin instead?

179

llvm::drop_begin ?

196

Using eraseArgument here is going to result in updating the indices multiple times. Also can you merge indice updates into this loop?

mehdi_amini added inline comments.Feb 26 2021, 4:39 PM
mlir/lib/IR/Block.cpp
196

Using eraseArgument here is going to result in updating the indices multiple times

Good point!

Also can you merge indice updates into this loop?

I'm not sure how though: the loop is going backward, you can't know on the first element you're deleting what its final index will be?
(at least not easily)

mehdi_amini marked 4 inline comments as done.

Address comments

rriddle accepted this revision.Feb 26 2021, 5:59 PM
rriddle added inline comments.
mlir/lib/IR/Block.cpp
160

nit: ++index; (You could also merge it into the drop_begin call)

196

I'm not sure how though: the loop is going backward, you can't know on the first element you're deleting what its final index will be?

(at least not easily)
Ah right, missed that this was in reverse.

208

You could use eraseIndices.find_first() to get the first removed argument.

This revision is now accepted and ready to land.Feb 26 2021, 5:59 PM
mehdi_amini marked 3 inline comments as done.Feb 27 2021, 9:09 AM
mehdi_amini added inline comments.Feb 27 2021, 9:12 AM
mlir/lib/IR/Block.cpp
208

Updated, but I'm not totally sure it's better for the common case: the code in find_first is not trivial compared to the vector traversal here.

Address comments

This revision was landed with ongoing or failed builds.Feb 27 2021, 9:21 AM
This revision was automatically updated to reflect the committed changes.