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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added a comment.

In https://reviews.llvm.org/D42251#993304, @rovka wrote:

> 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.


We could also fix that inconsistency by checking for a single element initializer list. I'll see what effect moving everything to the initializer_list has. I doubt it's a measurable difference and if that's the case I'll resolve the inconsistency that way. Otherwise, I'll make it so the initializer_list variant only marks it an alias if you give it multiple opcodes.

>   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.

Sure, I'll add something to getActionDefinitionsBuilder() about it.

> Hope that helps,
> Diana



================
Comment at: llvm/trunk/lib/Target/AArch64/AArch64LegalizerInfo.cpp:243
+  }
+  getActionDefinitionsBuilder(G_ATOMIC_CMPXCHG);
 
----------------
rovka wrote:
> This doesn't look like it belongs here...
Well spotted. This used to have a `.unsupported()` on the end. I must have missed it when I switched falling off the end of the rules to mean unsupported.


Repository:
  rL LLVM

https://reviews.llvm.org/D42251