This is an archive of the discontinued LLVM Phabricator instance.

AMDGPU: Avoid folding 2 constant operands into an SALU operation
ClosedPublic

Authored by dstuttard on Dec 2 2019, 5:06 AM.

Details

Summary

Catch the (admittedly unusual) case where SIFoldOperands attempts to fold 2
constant operands into the same SALU operation, with neither operand able to be
encoded as an inline constant.

Change-Id: Ibc48d662c9ffd8bbacd154976b0b1c257ace0927

Event Timeline

dstuttard created this revision.Dec 2 2019, 5:06 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 2 2019, 5:06 AM
dstuttard updated this revision to Diff 231681.Dec 2 2019, 5:07 AM

Formatting

I would have preferred to have put this check into isImmOperandLegal in SIInstrInfo.cpp - but that produced lots of lit regressions. Looks like commute operations use this function even when they are swapping rather than replacing the operand (which breaks).

arsenm added a comment.Dec 2 2019, 8:02 AM

I've never completely liked the way isOperandLegal is structured to always check the legality with respect to all other operands.

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
437

TRI is already a class member

440

Demorgan this, and I would order TRI.opCanUseInlineConstant(OpInfo.OperandType) before isInlineConstant

445–446

This isn't the right check for a non-inline constant. It won't catch frame index for example.

llvm/test/CodeGen/AMDGPU/fold-sgpr-multi-imm.mir
13

Is this just hiding not handling every possible case in tryConstantFoldOp? Can you add a test with a more exotic instruction we're unlikely to ever try to constant fold here?

Another case that should always break is a frame index operand

dstuttard updated this revision to Diff 231860.Dec 3 2019, 3:26 AM
dstuttard marked 6 inline comments as done.

Made suggested changes

llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
437

I couldn't work out if you were objecting to the name "TRI" or if you wanted me to re-use TRI from the class object.
I decided to rename it to SRI since to re-use TRI requires a more pervasive change and an extra cast as well.

llvm/test/CodeGen/AMDGPU/fold-sgpr-multi-imm.mir
13

I wasn't sure if the instruction I've chosen is now obscure enough - give me a different suggestion if you can think of something better. (Also added the frameindex case).

arsenm added inline comments.Dec 3 2019, 9:00 AM
llvm/lib/Target/AMDGPU/SIFoldOperands.cpp
437

I mean re-use. I'm surprised a non-target reference to TRI exists anywhere in this pass

arsenm accepted this revision.Dec 3 2019, 9:04 AM
This revision is now accepted and ready to land.Dec 3 2019, 9:04 AM
This revision was automatically updated to reflect the committed changes.