[PATCH] D70000: [DAGCombine] Initialize the default operation action for SIGN_EXTEND_INREG for vector type as 'expand' instead of 'legal'

[qshanz via Phabricator via llvm-commits]

steven.zhang added a comment.

In D70000#1741155 <https://reviews.llvm.org/D70000#1741155>, @dmgreen wrote:

> In D70000#1740595 <https://reviews.llvm.org/D70000#1740595>, @steven.zhang wrote:
>
> > In D70000#1740485 <https://reviews.llvm.org/D70000#1740485>, @dmgreen wrote:
> >
> > > It seems that this thing has been around since 2010. How come we are changing the default now, as opposed to just fixing the backends that don't support this instruction?
> > >
> > > If MIPs is the target that is failing, there should probably be a test to show it is now OK.
> >
> >
> > Yeah, it could be easy to fix those target that didn’t set as expand. But it is just hiding the problem. The key here is that, for sext_inreg for vector type, the default action should be expand instead of legal, just as what we did for sext_vector_inreg. See the test I gave on hexagon target(we fail to do the instr selection). It is easy to cook this test and expose the problem. I believe we might have more potential issues as more targets are added. The problem of hexagon is exactly the same as mips, as we are marking those op that didn’t have native instruction support as legal.
>
>
> Righteo. And your argument for changing the default at this late stage is for consistency with the other _inreg nodes? That sounds fair. The counter argument would be that it is also nice to keep consistency with what was already in llvm! Not make changes like this that change the default action for an operation unless we have good reason to do so.
>
> Take something like FLOG10 for example. It defaults to legal and targets should be making it Expand if they do not have a relevant instruction, like most targets do. Something like FPOWI will default to Expand though, because it was added later and would break existing targets if it was not Expand. There doesn't seem to be a lot of consistency other than when the node was added.
>
> I don't really have a strong opinion. If people think that changing the default is good for consistency with the other _inreg nodes, then that's fine.


Agree with you on that, keeping the consistency is NOT the reason. We need to figure out the correct way.

In llvm, **we have 18 targets now. But only 2 targets support the sext_inreg for vector type**! That is, ARM and Hexagon. Even within these two targets, it only supports the operation for some limited vector type. We might have:

1. sext_inreg for vector type is legal by default.
  - That requires all the targets to mark it as expand if they didn't have the native instruction support. However, **most targets didn't do this.** And bugs hide. i.e. Hexagon failed to select the instruction. And I believe it is easy to cook the test for most target to expose the issue. And as you mentioned, we could easily fix this by visit all the targets and set it as expand. If we did that, why cannot we just mark it as expand by default ?
2. sext_inreg for vector type is expand by default
  - What we need to do, is set it as legal for some vector type for ARM and Hexagon.

The option 2 makes more sense to me, as it is consistent with the semantics. The op is NOT legal for most platform currently. And that is also what we have done for sext_vector_inreg. My understanding is that, **if this operation is NOT supported by most targets, we'd better set it as expand by default**. Please correct me if I miss something. And I agree that, sometimes, we are not willing to change the default value which might break the other targets. But that is not the reason not to do it ... What do you think ?


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D70000/new/

https://reviews.llvm.org/D70000