[PATCH] D38200: [GISel]: Process new insts to legalize in the order they were created

[Kristof Beyls via Phabricator via llvm-commits]

kristof.beyls added a comment.

> While this shouldn't be an issue (legalization should only look at one instruction), there's some out of tree code that looks at other instructions as well, and this is breaking the legalization for that instruction.

I'm a bit confused by the above. Should we allow legalization to look at other instructions, or should we not?



================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:55
 
+struct WorkListType {
+  WorkListType() = default;
----------------
I don't think I've seen the naming convention of ending type names with "Type" in the LLVM code base - but I also by far haven't looked at all the LLVM code.
Maybe "struct WorkList" would be better.

I find it unfortunate that we would need to create a custom container here for this single specific use case. Is there really not a pre-existing container that is good enough? My guess is that this WorkListType's advantage over a deque is only that it can check quickly if an element is present in the container. Is that really such a frequent operation that that must be fast, but removing that element if present doesn't have to be faster than deque.erase?


================
Comment at: lib/CodeGen/GlobalISel/Legalizer.cpp:94
+  std::deque<MachineInstr *> Q;
+  std::set<MachineInstr *> S;
+};
----------------
unordered_set could be used instead of set?


Repository:
  rL LLVM

https://reviews.llvm.org/D38200