Page MenuHomePhabricator

[InstCombine] Handle select inst when eliminating constant memcpy
ClosedPublic

Authored by gandhi21299 on Oct 22 2022, 3:14 AM.

Details

Summary

Allow iterating through SelectInst use of the alloca when
checking if it is only ever overwritten from constant memory.
Recursively determine if the SelectInst is replacable and insert
it into the Worklist if so. Finally, define a new SelectInst to
replace the old one, with both of it's values replaced according
to the WorkMap.

Diff Detail

Event Timeline

gandhi21299 created this revision.Oct 22 2022, 3:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2022, 3:14 AM
Herald added a subscriber: hiraditya. · View Herald Transcript
gandhi21299 requested review of this revision.Oct 22 2022, 3:14 AM
Herald added a project: Restricted Project. · View Herald TranscriptOct 22 2022, 3:14 AM
  • removed #include "llvm/IR/Instruction.h"
  • ran update_test_checks.py on the test
arsenm added inline comments.Nov 10 2022, 11:48 AM
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
404

IC.InsertNewInstWith should take care of debug info, but you're dropping any other instruction metadata that was attached

gandhi21299 planned changes to this revision.Nov 14 2022, 11:23 AM
gandhi21299 updated this revision to Diff 490302.EditedWed, Jan 18, 2:45 PM
  • Rebased against the latest revision.
  • Check if the true and false values of SelectInst are already in the Worklist before inserting the SelectInst itself into the Worklist.
  • Define a new SelectInst while preserving the attached MetaData.
  • Revise tests
gandhi21299 edited the summary of this revision. (Show Details)Wed, Jan 18, 2:45 PM
gandhi21299 added inline comments.Wed, Jan 18, 3:48 PM
llvm/test/Transforms/InstCombine/ptr-replace-alloca.ll
434

Need to be erased since it's never used.

nikic requested changes to this revision.Thu, Jan 19, 2:03 AM
nikic added inline comments.
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
82

Handling for SelectInst should use same logic as PHINode (with IsOffset = true). Can you please add a test with a memcpy on the select result, which should not get transformed?

This revision now requires changes to proceed.Thu, Jan 19, 2:03 AM
gandhi21299 marked 2 inline comments as done.
  • isOffset = true for SelectInst in isOnlyCopiedFromConstantMemory(..)
  • Added a test where memcpy is performed upon the result of a SelectInst
  • Removed the intrinsic in the test which is never used.
nikic added a comment.Thu, Jan 19, 5:38 AM

This is missing a test with different address space and removed alloca, but otherwise looks fine.

This is missing a test with different address space and removed alloca, but otherwise looks fine.

Is there one? I can't think of a test case where the source and dest of a memcpy are in different address spaces and the alloca is removed.

nikic added a comment.Thu, Jan 19, 6:47 AM

This is missing a test with different address space and removed alloca, but otherwise looks fine.

Is there one? I can't think of a test case where the source and dest of a memcpy are in different address spaces and the alloca is removed.

A select between alloca and gep of alloca should work.

  • Added a test with varying addrspaces across the source and dest of a memcpy and the alloca is eliminated.
nikic added inline comments.Thu, Jan 19, 7:36 AM
llvm/test/Transforms/InstCombine/ptr-replace-alloca.ll
375

All of these 256 should be 32 instead.

423–425

Otherwise the select is simply unused, and this doesn't show the desired transform.

gandhi21299 marked an inline comment as done.Thu, Jan 19, 8:28 AM
gandhi21299 added inline comments.
llvm/test/Transforms/InstCombine/ptr-replace-alloca.ll
423–425

Despite the change from 256 to 32 and your comment above, I stil get the same result. Some other instcombine optimization optimizes this function out apparently.

  • Copy 32 bytes instead of 256 bytes.
nikic added a comment.Fri, Jan 20, 2:54 AM

Can you please rebase over https://github.com/llvm/llvm-project/commit/f61ee94470439c54499d43f0826cee04d99d5e9b? I think this should work. The problem was that we convert load of select into select of loads, so the select is outside the transform. Adding an extra GEP afterwards avoid this.

Something went wrong with the rebase. This should be the ptr-replace-alloca.ll from main with update_test_checks rerun.

Inherit changes which were unintentionally eliminated during rebase.

Run update_test_checks on ptr-replace-alloca.ll

nikic added inline comments.Mon, Jan 23, 5:17 AM
llvm/lib/Transforms/InstCombine/InstCombineLoadStoreAlloca.cpp
329

Okay, I understand now why the new test case does not fold. Please switch this to the isAvailable() method added in https://github.com/llvm/llvm-project/commit/ed9806363beb1caf5265e75a64de904b60218093 and regenerate the test. I think then everything should work as expected.

  • use isAvailable as requested by reviewer
  • update test accordingly
gandhi21299 marked an inline comment as done.Mon, Jan 23, 8:47 AM

Refreshing patch

nikic accepted this revision.Mon, Jan 23, 8:55 AM

LGTM

This revision is now accepted and ready to land.Mon, Jan 23, 8:55 AM

Thank you for the review Nikita, I will commit this patch as soon as the pre-merge checks complete.

This revision was landed with ongoing or failed builds.Mon, Jan 23, 11:03 AM
This revision was automatically updated to reflect the committed changes.