[GlobalISel] Add dead flags to implicit defs in ISel

[Pierre-vh]

Summary

Checks for implicit defs that are unused within a pattern and mark them as dead.

This is done directly at the TableGen level forr efficiency.
The instructions are directly created with the "dead" operand and no further analysis is needed later.

[Pierre-vh]

A couple of things to note:

• This is a bit naive, it only works on the Record* and doesn't test for subregs or RC, should it?
• One of the tradeoffs of doing this at the TableGen level is that we don't know about reserved registers. Do they matter in this case? How could we handle them?

[foad]

Are there any cases where this improves codegen?

[Pierre-vh]

In D157273#4565239, @foad wrote:

Are there any cases where this improves codegen?

The only codegen change I spotted was in mul-scalar.ll (X86)
The intent is just to improve correctness. @arsenm noticed that not setting dead flags was an issue in https://github.com/llvm/llvm-project/issues/63986

[arsenm]

In D157273#4565254, @Pierre-vh wrote:

In D157273#4565239, @foad wrote:

Are there any cases where this improves codegen?

The only codegen change I spotted was in mul-scalar.ll (X86)
The intent is just to improve correctness. @arsenm noticed that not setting dead flags was an issue in https://github.com/llvm/llvm-project/issues/63986

It's not really a correctness fix, it's just avoiding regressing > 30 globalisel tests when my fix for that is applied. Most of the cases we're compensating for missing constant folding in GlobalISel

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h

335

Can just make the flag value?

335

actually you can compact the two into one field, register is 32-bits and the flags are 16 I think

390–392

Unrelated formatting change

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h

951–952

I'd expect the table entry to just be any flags an unconditionally or'd

[arsenm]

In D157273#4565233, @Pierre-vh wrote:

A couple of things to note:

• This is a bit naive, it only works on the Record* and doesn't test for subregs or RC, should it?

Probably can ignore this

• One of the tradeoffs of doing this at the TableGen level is that we don't know about reserved registers. Do they matter in this case? How could we handle them?

It doesn't matter, dead flags shouldn't be set on reserved registers but also shouldn't matter

[Pierre-vh]

Comments

[Pierre-vh]

In D157273#4567283, @arsenm wrote:

In D157273#4565233, @Pierre-vh wrote:

A couple of things to note:

• This is a bit naive, it only works on the Record* and doesn't test for subregs or RC, should it?

Probably can ignore this

• One of the tradeoffs of doing this at the TableGen level is that we don't know about reserved registers. Do they matter in this case? How could we handle them?

It doesn't matter, dead flags shouldn't be set on reserved registers but also shouldn't matter

I guess if we want to be 100% correct we could check for the presence of RegState::Dead in the flags, and check if we're dealing with a reserved register.
If it doesn't matter though let's leave it at that, it can always be added later.

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h

335

I think compacting it is more trouble than it's worth, and it'd make this opcode an outlier - e.g. AddTempRegister doesn't compact, AddTempSubRegister doesn't, etc.

We should just look into making this a variable-length encoded array at some point, but it's quite a bit of work. Probably not worth it until we get actual performance numbers.

I wonder if we ever make use of the upper 32 bits though, maybe the table should always be 32 bits, with an option to variable-length-encode bigger values.

[arsenm]

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h

335

could change all these easy cases at once

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h

950–951

This could have just been in the table