This is an archive of the discontinued LLVM Phabricator instance.

[GlobalISel] New combine to commute constant operands to the RHS
ClosedPublic

Authored by foad on Jan 3 2023, 9:26 AM.

Diff Detail

Event Timeline

foad created this revision.Jan 3 2023, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2023, 9:26 AM
foad requested review of this revision.Jan 3 2023, 9:26 AM
Herald added a project: Restricted Project. · View Herald TranscriptJan 3 2023, 9:26 AM

This achieves some of the same multiply-by-zero cleanups as D140208.

arsenm accepted this revision.Jan 3 2023, 9:30 AM
arsenm added a subscriber: arsenm.
arsenm added inline comments.
llvm/include/llvm/Target/GlobalISel/Combine.td
348

Should also get compares, but that's trickier since you have to swap the predicate

360

Should also get the FP cases

This revision is now accepted and ready to land.Jan 3 2023, 9:30 AM
OutOfCache added inline comments.Jan 5 2023, 12:55 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll
53

@tsymalla suggested in my revision to give the test cases more descriptive names.

258

This is a neat approach! Is there a possibility to extend this to G_MAD instructions as well? It's trickier since the operands don't have the same indices as for G_MUL etc. though.

312

G_MAD does not take advantage of the binop_right_to_zero rule.

386
432

LGTM

llvm/include/llvm/Target/GlobalISel/Combine.td
349

This looks alright to me, but what is the point in swapping the operands if both of them are constants except making the ISA more readable?
For instance:

s_add_i32 s1, 0x1000, 0 => s_add_i32 s1, 0, 0x1000

llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll
174

Shouldn't this one be eliminated?

foad added inline comments.Jan 5 2023, 3:04 AM
llvm/include/llvm/Target/GlobalISel/Combine.td
349

If both operands are constants then all the opcodes will be constant-folded.

llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll
53

That's fine but I think it should be a separate patch.

174

Why? It's used by the global_store below.

258

There are no generic G_MAD instructions, so I think we would need to add a target-specific combine for target-specific opcodes.

312

Likewise, I think that would need to be a target-specific combine.

tsymalla added inline comments.Jan 5 2023, 3:06 AM
llvm/test/CodeGen/AMDGPU/GlobalISel/mul-known-bits.i64.ll
174

Missed that one.

This revision was landed with ongoing or failed builds.Jan 5 2023, 3:13 AM
This revision was automatically updated to reflect the committed changes.