This is an archive of the discontinued LLVM Phabricator instance.

[InstSimplify] Expose generic interface for replaced operand simplification
ClosedPublic

Authored by jdoerfert on Jul 16 2021, 2:08 PM.

Details

Summary

Users, especially the Attributor, might replace multiple operands at
once. The actual implementation of simplifyWithOpReplaced is able to
handle that just fine, the interface was simply not allowing to replace
more than one operand at a time. This is exposing a more generic
interface without intended changes for existing code.

Diff Detail

Event Timeline

jdoerfert created this revision.Jul 16 2021, 2:08 PM
jdoerfert requested review of this revision.Jul 16 2021, 2:08 PM
Herald added a reviewer: baziotis. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript
jdoerfert updated this revision to Diff 359445.Jul 16 2021, 2:10 PM

Rename function, improve documentation, clang format

lebedev.ri added inline comments.Jul 16 2021, 2:11 PM
llvm/include/llvm/Analysis/InstructionSimplify.h
328–333
lebedev.ri added inline comments.Jul 16 2021, 2:16 PM
llvm/include/llvm/Analysis/InstructionSimplify.h
332

Should this be ArrayRef?

jdoerfert updated this revision to Diff 359450.Jul 16 2021, 2:23 PM
jdoerfert marked 2 inline comments as done.

Use ArrayRef

nikic added a comment.Jul 16 2021, 2:28 PM

I don't think simplifyWithOpReplaced() is the right starting point for this. This function has a specific purpose and it has complex and dangerous semantics.

I think what you really want is something along the lines of ConstantFoldInstruction vs ConstantFoldInstOperands. That is, you should take SimplifyInstruction and rewrite it into a SimplifyInstOperands function that takes the operands as an explicit ArrayRef, and then call that from SimplifyInstruction. That will ensure that you can actually simplify all instructions that InstSimplify supports, rather than just the peculiar subset that simplifyWithOpReplaced deals with.

lebedev.ri accepted this revision.Jul 16 2021, 2:30 PM

Seems straight-forward to me

This revision is now accepted and ready to land.Jul 16 2021, 2:30 PM
jdoerfert planned changes to this revision.Jul 16 2021, 2:54 PM

Working on the alternative

llvm/include/llvm/Analysis/InstructionSimplify.h
332

Yes.

jdoerfert updated this revision to Diff 359464.Jul 16 2021, 3:39 PM

Change approach to use the SimplifyInstruction interface instead. Tests not updated yet.

This revision is now accepted and ready to land.Jul 16 2021, 3:39 PM

@nikic Like this?

I needed to use NewOps for load simplification to make it as powerful as the former approach. Also did the same to PHI simplification because it seemed easy.

simplifyInstruction*With*Operands

nikic accepted this revision.Jul 17 2021, 3:23 AM

LG

llvm/lib/Analysis/InstructionSimplify.cpp
6051–6055

make the load

6081

This fallback shouldn't be necessary, ConstantFoldInstruction bails on non-constant operands.

jdoerfert updated this revision to Diff 359921.Jul 19 2021, 2:32 PM
jdoerfert marked 2 inline comments as done.

Address comments

lebedev.ri accepted this revision.Jul 19 2021, 3:01 PM

Not sure if it makes sense to commit some parts of this separately, maybe not.

llvm/lib/Analysis/InstructionSimplify.cpp
6221–6222

assert(NewOps.size() == I.getNumOperands() && "");

jdoerfert updated this revision to Diff 359933.Jul 19 2021, 3:07 PM
jdoerfert marked an inline comment as done.

Add assert

This revision was landed with ongoing or failed builds.Jul 26 2021, 10:57 PM
This revision was automatically updated to reflect the committed changes.