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

[Daniel Sanders via Phabricator via llvm-commits]

dsanders added inline comments.


================
Comment at: include/llvm/CodeGen/GlobalISel/LegalizerInfo.h:371-374
+  LegalizeRuleSet &fallback() {
+    add({always, LegalizeAction::UseLegacyRules});
+    return *this;
+  }
----------------
bogner wrote:
> dsanders wrote:
> > bogner wrote:
> > > I'm not sure we want this here - this would allow someone to write some new style rules for an op but then fallback as well, which is just confusing. I think we should enforce either new style rules or fallback, and not allow weird combinations of both.
> > This was intended to be a migration tool but it's not actually in use anymore. I'll remove it.
> > 
> > At the moment, if you fall off the end of the ruleset (which is prevented by the `.unsupported()` calls) then it will fall back on the old implementation. This lets you implement the `.legal*()` part of the rule first and then figure out how to write the legalization rules. I want to push towards deleting the old implementation as soon as possible.
> Right. I'd rather falling off the end of the ruleset mean unsupported, and I don't really see a reason to allow fallback for an op once you've started writing the rules for that op.
Ok, I can switch it over to that


================
Comment at: lib/Target/AArch64/AArch64LegalizerInfo.cpp:53
+      .widenScalarToPow2(0, 8)
+      .unsupported();
 
----------------
bogner wrote:
> dsanders wrote:
> > bogner wrote:
> > > Would it make more sense to have the end of the list be implicitly unsupported? I think currently it would go to fallback instead, but as I pointed out earlier I don't think that's the right thing to do.
> > In the long run, yes. Once we've all moved to this then we'll be able to make it the default. Until then, we need a way to fallback on the existing implementation when there are no rules specified or when they are incomplete
> I don't understand when having incomplete rules that lead to fallback would be useful. I'm reasonably sure it would make more sense to say "no rules specified implies fallback, but any rules specified implies that the rules are complete". People would still be able to write the new rules for an opcode at a time, so we still wouldn't have to do an all or nothing switch.
It's not useful for very long but it's handy when writing the rules. The way I've been porting these is:
* Add the legal cases and test it
* Add the scalar legalization cases and test it
* Add the vector legalization cases and test it
* Tack unsupported() on the end, delete the old code and test it once more.
At each step, the old code handles any cases I haven't dealt with yet


https://reviews.llvm.org/D42251