Page MenuHomePhabricator

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

Authored by steven.zhang on Nov 8 2019, 2:53 AM.

Details

Summary

For now, we didn't set the default operation action for SIGN_EXTEND_INREG for vector type, which is 0 by default, that is legal. However, most target didn't have native instructions to support this opcode. It should be set as expand by default, as what we did for ANY_EXTEND_VECTOR_INREG.

This become critical as some target(i.e. Mips) didn't set it as expand for vector type, which will have problems as they didn't have the native instruction for it.

This patch will set the default operation action for SIGN_EXTEND_INREG for vector type as expand. And grep all the targets that specify its match pattern for vector type and make it legal.

Diff Detail

Event Timeline

steven.zhang created this revision.Nov 8 2019, 2:53 AM
jsji added a subscriber: jsji.EditedNov 8 2019, 8:28 AM

Why do you call this [NFC]? Also why you think this do NOT need any testcases?

Why do you call this [NFC]? Also why you think this do NOT need any testcases?

This patch just change the default action operation from legal to expand, and mark it as legal for each target that supports it. It is nfc in essential. And I agree that, it might change the action from legal to expand for those target mis-set it before.

steven.zhang planned changes to this revision.Nov 8 2019, 6:10 PM

And yes, I need add test as it indeed change the behavior.

steven.zhang retitled this revision from [DAGCombine][NFC] Initialize the default operation action for SIGN_EXTEND_INREG for vector type as 'expand' instead of 'legal' to [DAGCombine] Initialize the default operation action for SIGN_EXTEND_INREG for vector type as 'expand' instead of 'legal'.Nov 8 2019, 6:10 PM

Add the tests.

steven.zhang added a comment.EditedNov 11 2019, 2:11 AM
  • llvm/test/CodeGen/ARM/signext-inreg.ll
    • Expose a bug. Because the default action is legal, we have to set it as expand inside addTypeForNEON. And it will override the action set for MVE. Change it to expand by default, and set it to legal for MVE.
  • CodeGen/Hexagon/signext-inreg.ll
    • Without this patch, we fail to select the instruction for test3 with option -march=hexagon. This is because all the sext_inreg for the vector type is legal by default if the target didn't set it. And Hexagon is the target. It will generate the sext_inreg node that target didn't support, which hit the assertion in the instruction selection. And this is exactly the case that this patch want to avoid.

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.

llvm/lib/Target/ARM/ARMISelLowering.cpp
382

Nice one.

llvm/test/CodeGen/ARM/signext-inreg.ll
2

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

steven.zhang added a comment.EditedNov 11 2019, 5:24 AM

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.

steven.zhang marked an inline comment as done.Nov 11 2019, 5:28 AM
steven.zhang added inline comments.
llvm/test/CodeGen/ARM/signext-inreg.ll
2

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!

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.

llvm/test/CodeGen/ARM/signext-inreg.ll
2

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.

steven.zhang added a comment.EditedNov 11 2019, 9:29 PM

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. As little target provides the match pattern, it is a good time to fix it after nearly ten years. What do you think ?

steven.zhang marked an inline comment as done.Nov 11 2019, 9:32 PM
steven.zhang added inline comments.
llvm/test/CodeGen/ARM/signext-inreg.ll
2

Thank you for the explanation, dmgree:) I will take a deep look on this.

steven.zhang marked an inline comment as done.Nov 11 2019, 11:57 PM
steven.zhang added inline comments.
llvm/test/CodeGen/ARM/signext-inreg.ll
2

So, we have ARM and Thumb2 and MVE is for Thumb2. Is it right ? I will add the test into mve-sext.ll. The testing here is trying to verify that, the Neon won't override the action operation setting for MVE.

dmgreen added inline comments.Nov 13 2019, 6:45 AM
llvm/test/CodeGen/ARM/signext-inreg.ll
2

Having Neon + MVE at the same time would be an invalid combination.

steven.zhang planned changes to this revision.Nov 13 2019, 6:51 AM
steven.zhang marked an inline comment as done.
steven.zhang added inline comments.
llvm/test/CodeGen/ARM/signext-inreg.ll
2

ok,get it.Thank you for the clarify.

Remove the NEON + MVE test, and add one test in mve-sext.ll, which will fail without this patch.

LLVM ERROR: Cannot select: t13: v4i32 = sign_extend_inreg t3, ValueType:ch:v4i1
  t3: v4i32 = bitcast t2
    t2: v2f64,ch = CopyFromReg t0, Register:v2f64 %0
      t1: v2f64 = Register %0
In function: sext_v4i32_v4i32

The reason is the same as Hexagon, Mips and other targets. That is, we are setting the default operation action for sext_inreg for vector type as legal.

OK. So it's not about consistency with the other INREGs, its to do with them rarely being used for vectors?

I think you reasoning makes a lot of sense. Plus anyone using these nodes downstream and relying on it being Legal will likely have tests for the sext_inreg patterns they added, so should easily catch any differences.

If the hexagon changes are OK here, I'm happy.

llvm/lib/Target/ARM/ARMISelLowering.cpp
189

You can leave this line in. It's not bad to be explicit about this being expand.

llvm/test/CodeGen/Thumb2/mve-sext.ll
4

Can you add equivalents for v8i16, v16i8 and maybe v2i64?

Or actually, I can do it later. I don't want to give you more work to do.

steven.zhang marked 2 inline comments as done.Nov 14 2019, 2:42 PM
steven.zhang added inline comments.
llvm/lib/Target/ARM/ARMISelLowering.cpp
189

make sense

llvm/test/CodeGen/Thumb2/mve-sext.ll
4

I will add it.

Address dmgreen's comments.

@kparzysz Would you please help me review the Hexagon's part change ?

kparzysz added inline comments.Nov 21 2019, 10:56 AM
llvm/lib/Target/Hexagon/HexagonISelLowering.cpp
1541

The types should be v2i8, v4i8, and v2i16.

llvm/lib/Target/Hexagon/HexagonISelLoweringHVX.cpp
23

Please do not use globals for this.

61

Please remove this.

199

Please replace this with

if (Use64b) {
  for (MVT T : {MVT::v16i8, MVT::v32i8, MVT::v16i16})
    setOperationAction(ISD::SIGN_EXTEND_INREG, T, Legal);
} else {
  for (MVT T : {MVT::v32i8, MVT::v64i8, MVT::v32i16})
    setOperationAction(ISD::SIGN_EXTEND_INREG, T, Legal);
}
steven.zhang marked 2 inline comments as done.

Address comments.

llvm/lib/Target/Hexagon/HexagonISelLowering.cpp
1541

I just only see two match pattern for this ISD Node in the TD file, and no match pattern for v4i8. Did I miss something ?

def: Pat<(v2i32 (sext_inreg V2I32:$Rs, v2i8)),
         (Combinew (A2_sxtb (HiReg $Rs)), (A2_sxtb (LoReg $Rs)))>;

def: Pat<(v2i32 (sext_inreg V2I32:$Rs, v2i16)),
         (Combinew (A2_sxth (HiReg $Rs)), (A2_sxth (LoReg $Rs)))>;
llvm/lib/Target/Hexagon/HexagonISelLoweringHVX.cpp
199

We need to mark the dest type as Legal too, as Vector Op Legalizer will check the operand 0 instead of the Operand 1 is legal or not. That is important for some target to support:

v4i32 sext_inreg Val, v4i8
v4i16 sext_inreg Val, v4i8

Gentle ping ...

@kparzysz Any more comments ? Thank you.

RKSimon accepted this revision.Jan 1 2020, 1:53 AM

LGTM

This revision is now accepted and ready to land.Jan 1 2020, 1:53 AM
steven.zhang updated this revision to Diff 235823.EditedJan 1 2020, 9:37 PM

Rebase the patch due to the AArch64 commit 65651f197a2c which try to legalize sext_inreg for vector type for SVE. We need to specify it explicitly instead of relying on the default value. I will hold on this patch for several days in case someone has comments on the change of "SVE" part.

dmgreen accepted this revision.Jan 2 2020, 1:35 AM

The AArch64 changes look good to me. Thanks

This revision was automatically updated to reflect the committed changes.