This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] Add dead flags to implicit defs in ISel
ClosedPublic

Authored by Pierre-vh on Aug 7 2023, 4:00 AM.

Details

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.

Diff Detail

Event Timeline

Pierre-vh created this revision.Aug 7 2023, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2023, 4:00 AM
Pierre-vh requested review of this revision.Aug 7 2023, 4:00 AM
Herald added a project: Restricted Project. · View Herald TranscriptAug 7 2023, 4:00 AM

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 added a comment.Aug 7 2023, 4:04 AM

Are there any cases where this improves codegen?

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 added a comment.Aug 7 2023, 9:37 AM

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–391

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 added a comment.Aug 7 2023, 2:15 PM

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 updated this revision to Diff 548132.Aug 8 2023, 2:48 AM
Pierre-vh marked 4 inline comments as done.

Comments

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 accepted this revision.Aug 8 2023, 4:17 AM
arsenm added inline comments.
llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutor.h
335

could change all these easy cases at once

llvm/include/llvm/CodeGen/GlobalISel/GIMatchTableExecutorImpl.h
950–953

This could have just been in the table

This revision is now accepted and ready to land.Aug 8 2023, 4:17 AM
This revision was automatically updated to reflect the committed changes.