[PATCH] D42251: [globalisel][legalizer] Adapt LegalizerInfo to support inter-type dependencies and other things.

Wed Jan 31 07:14:59 PST 2018

rovka added a comment.

Hi,

I've been playing a bit with this for ARM (r323876) and it's been pretty straightforward to update. I have a few observations that I thought I could share, but feel free to ignore them depending on how you intend to evolve this in the future.

First of all, there's an inconsistency between getActionDefinitionsBuilder(Opcode) and getActionDefinitionsBuilder({Opcode}) - the latter marks Opcode as an alias even though there's nothing to alias with. Because of this, we get the somewhat surprising behaviour that using the former it's possible to add more rules even without saving the builder, whereas using the latter asserts. I'm not a huge fan of extra characters, but maybe we should just get rid of the first variant? This would be consistent with the rest of the API, since you can say legalFor({Type}), but not legalFor(Type).  Of course, we could also go the other way and get rid of even more {} by using variadic templates instead of initializer lists (except for places where it looks good, like the CartesianProduct APIs). I haven't thought too much about it, so I'm not sure if it's worth the overhead, I'm just throwing it out as a potential alternative.

Also related to aliasing, I think some of the discussions from this review should move into comments in the code, because just by looking around it's a bit unclear if it's just an implementation detail / optimization or something users of the API should care about. In particular, the behaviour relative to progressively adding new rules to one or more opcodes should probably be documented somehow.

Hope that helps,
Diana

================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64LegalizerInfo.cpp:243
+  }
+  getActionDefinitionsBuilder(G_ATOMIC_CMPXCHG);

----------------
This doesn't look like it belongs here...

Repository:
  rL LLVM

https://reviews.llvm.org/D42251