This is an archive of the discontinued LLVM Phabricator instance.

[mlir][bytecode] Avoid recording null arglocs & realloc opnames.
ClosedPublic

Authored by jpienaar on May 20 2023, 2:03 PM.

Details

Summary

For block arg locs a common case is no/uknown location (where the producer
signifies they don't care about blockarg location). Also avoid needing to
dynamically resize opnames during parsing.

Assumed to be post lazy loading change, so chose version 3.

Diff Detail

Event Timeline

jpienaar created this revision.May 20 2023, 2:03 PM
Herald added a project: Restricted Project. · View Herald TranscriptMay 20 2023, 2:03 PM
jpienaar requested review of this revision.May 20 2023, 2:03 PM
mehdi_amini added inline comments.May 20 2023, 8:48 PM
mlir/lib/Bytecode/Reader/BytecodeReader.cpp
2162–2163

This initialization seems unusual to me: isn't it the default?

2164

Can't this hasLoc be nested in the version >= 3 branch?

2167

I'm a bit puzzled: seems like this would be true is argType is valid? I don't quite get this condition.

2172

I don't quite get this "hasLoc" test here?

jpienaar updated this revision to Diff 525374.May 24 2023, 4:55 PM

Update to head & make logic a bit clearer for parsing.

rriddle added inline comments.May 24 2023, 5:10 PM
mlir/lib/Bytecode/Reader/BytecodeReader.cpp
2171–2178
jpienaar marked an inline comment as done.May 24 2023, 7:52 PM
jpienaar added inline comments.
mlir/lib/Bytecode/Reader/BytecodeReader.cpp
2162–2163

I made these a bit more explicit and used more local vars.

2172

I first misunderstood your question, then noticed it can be folded in.

jpienaar updated this revision to Diff 525414.May 24 2023, 7:56 PM

Simplify following suggestion

jpienaar marked 3 inline comments as done.May 24 2023, 7:57 PM
jpienaar added a reviewer: rriddle.
mehdi_amini accepted this revision.May 25 2023, 12:29 AM
This revision is now accepted and ready to land.May 25 2023, 12:29 AM
rriddle accepted this revision.May 25 2023, 12:32 AM