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

[Dave Green via Phabricator via llvm-commits]

dmgreen added a comment.

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.



================
Comment at: llvm/test/CodeGen/ARM/signext-inreg.ll:2
 ; NOTE: Assertions have been autogenerated by utils/update_llc_test_checks.py
 ; RUN: llc < %s -mtriple=armv8 -mattr=+mve | FileCheck %s
 define <4 x i32> @test(<4 x i32> %m) {
----------------
steven.zhang wrote:
> dmgreen wrote:
> > There are already tests in Thumb2/mve-sext.ll which cover sexts. The target here is a little odd for mve (mixing A profile and M profile architectures).
> I am not familiar with Arm target... From the implementation, it is valid case to show the issue. I will take a look at the case you mentioned. Thank you!
The ARM backend has two completely separate vector architectures in it. There is Neon, the older of the two that works with Cortex-A style cpus (think the big-little cores that are in you phone). Recently we have also added the MVE vector extension that works with Cortex-M style cpus, which are smaller microcontrollers designed to go anywhere (like sensors and other low-power devices). Cortex-A can be A32 (arm) or T32 (thumb), Cortex-M can only be T32 (thumb)

So this test has a Cortex-A device (running in A32 mode) with MVE (an T32 extension to M-class cpus).

I'm not sure this test is adding a huge amount for MVE, but if you want to keep it I would suggest that part should be moved to with the other MVE tests. Like I said though, they will already be tested by the mve-sext.ll tests. Leaving Neon here sounds good.


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

https://reviews.llvm.org/D70000