Page MenuHomePhabricator

Make TargetInstrInfo::foldMemoryOperand conditionally add memory operands
Needs ReviewPublic

Authored by prathamesh on Sep 13 2020, 8:51 PM.

Details

Reviewers
dmgreen
Summary

Hi,
While working on D79785, we wanted to define foldMemoryOperandImpl for Thumb1, that folds load, indirect call to direct call:
tLDRpci, tBLX -> tBL.

This triggered an assertion error with expensive checks turned on in MachineVerifier because the
newly created tBL insn by Thumb1InstrInfo::foldMemoryOperandImpl had memory operands of LoadMI
attached by TargetInstrInfo::foldMemoryOperand, which is done unconditionally:

// Copy the memoperands from the load to the folded instruction.
if (MI.memoperands_empty()) {
  NewMI->setMemRefs(MF, LoadMI.memoperands())

And ARM::tBL does not have mayLoad or mayLoadorStore set, since it doesn't require memory operands.
In the attached patch, I modified foldMemoryOperand to conditionally add memory operands to MI returned
by foldMemoryOperandImpl, if MI has mayLoad or mayLoadorStore property set.

This patch (along with D79785 patch) builds successfully with expensive checks enabled and passes check-llvm.
Does this patch look OK ?

Alternatively, Quentin suggested in http://lists.llvm.org/pipermail/llvm-dev/2020-September/144999.html,
to attach mayLoad to ARM::tBL, which will also resolve the issue.
Which approach is more preferable ?
Thanks.

Diff Detail

Event Timeline

prathamesh created this revision.Sep 13 2020, 8:51 PM
prathamesh requested review of this revision.Sep 13 2020, 8:51 PM
prathamesh edited the summary of this revision. (Show Details)