[PATCH] D87594: Make TargetInstrInfo::foldMemoryOperand conditionally add memory operands

[Prathamesh via Phabricator via llvm-commits]

prathamesh created this revision.
prathamesh added a reviewer: dmgreen.
Herald added subscribers: llvm-commits, hiraditya, kristof.beyls.
Herald added a project: LLVM.
prathamesh requested review of this revision.

Hi,
While working on D79785 <https://reviews.llvm.org/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 <https://reviews.llvm.org/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.


Repository:
  rG LLVM Github Monorepo

https://reviews.llvm.org/D87594

Files:
  llvm/lib/CodeGen/TargetInstrInfo.cpp


Index: llvm/lib/CodeGen/TargetInstrInfo.cpp
===================================================================
--- llvm/lib/CodeGen/TargetInstrInfo.cpp
+++ llvm/lib/CodeGen/TargetInstrInfo.cpp
@@ -603,7 +603,7 @@
     NewMI = foldMemoryOperandImpl(MF, MI, Ops, MI, FI, LIS, VRM);
   }
 
-  if (NewMI) {
+  if (NewMI && NewMI->mayLoadOrStore()) {
     NewMI->setMemRefs(MF, MI.memoperands());
     // Add a memory operand, foldMemoryOperandImpl doesn't do that.
     assert((!(Flags & MachineMemOperand::MOStore) ||
@@ -676,6 +676,9 @@
   if (!NewMI)
     return nullptr;
 
+  if (!NewMI->mayLoad())
+    return NewMI;
+
   // Copy the memoperands from the load to the folded instruction.
   if (MI.memoperands_empty()) {
     NewMI->setMemRefs(MF, LoadMI.memoperands());


-------------- next part --------------
A non-text attachment was scrubbed...
Name: D87594.291487.patch
Type: text/x-patch
Size: 772 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-commits/attachments/20200914/a61ba42a/attachment.bin>