Change prototype for TargetInstrInfo::foldMemoryOperandImpl

Hi,
While working on https://reviews.llvm.org/D79785, we wanted to define
foldMemoryOperandImpl hook 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())

In this case, we don't want the memory loads to be added to MI from
LoadMI. Should there be some mechanism for target specific
foldMemoryOperandImpl hook to signal to foldMemoryOperand to not add
memory operands from LoadMI ?

I was wondering if either of these approaches look good ?
(a) Make foldMemoryOperandImpl return two values via std::pair or use
OUT param (reference). The first is the MI instruction, and second a
bool whether or not to add memory operands from LoadMI. This will
however require adjusting other targets that override
foldMemoryOperandImpl. Would that be OK ?

(b) Define another method in TargetInstrInfo that decides whether or
not to add memory operands to the MI returned by
foldMemoryOperandImpl, which will not require to modify existing code,
but not sure if it's a good idea ?

(c) Make foldMemoryOperandImpl itself add memory operands, which will
require somewhat more complicated changes in targets that already
implement foldMemoryOperandImpl.

I would be grateful for suggestions on how to proceed.

Thanks,
Prathamesh

Hi Prathamesh,

What is the machine verifier error that you get?

I am wondering if the issue is not in the machine verifier itself in the sense that it is somewhat reasonable to have calls “document” which memory slot they access.

I am not fan of the idea of removing the addition of the memory operands because:

  1. This was part of the interface contract
  2. This may prevent unfolding to work

For #1 see that comment on TargetInstrInfo::foldMemoryOperandImpl:

/// Target-independent code in foldMemoryOperand will
/// take care of adding a MachineMemOperand to the newly created instruction.

For #2, I am guessing folding at one point and unfolding later is a rare enough thing that we may not care.

Cheers,
-Quentin

Hi Quentin,
I get following error from MachineVerifier:
# End machine code for function f.
*** Bad machine code: Missing mayLoad flag ***

which comes from:
// Check the MachineMemOperands for basic consistency.
  for (MachineMemOperand *Op : MI->memoperands()) {
    if (Op->isLoad() && !MI->mayLoad())
      report("Missing mayLoad flag", MI);
    if (Op->isStore() && !MI->mayStore())
      report("Missing mayStore flag", MI);
  }

The MI is ARM::tBL, and it doesn't have mayLoad set, which hits the
assert in MachineVerifier, since it still has memory operands attached
by foldMemoryOperand.
I have attached patch (changes to foldMemoryOperand), that checks if
the MI returned by foldMemoryOperandImpl has mayLoad or mayLoadOrStore
property set, and then proceed with adding memory operands, which
seems to resolve the issue.
Testing with make check-llvm with enable expensive checks doesn't show
unexpected failures.
Do the changes to foldMemoryOperand look OK ?

Thanks,
Prathamesh

foldmemoperand.diff (925 Bytes)

Would it make sense to attach mayLoad to ARM::tBL instead?

Hi,
I think that should also work to resolve the issue. However, ARM::tBL
is a direct call instruction requiring no memory operands,
and thus I am not sure if we should set mayLoad to workaround
foldMemoryOperand ? IIUC, the memory operand added
from LoadMI (callee's address) into ARM::tBL would be redundant.
Would it be OK instead to make foldMemoryOperand conditionally add
memory operands if the
MI returned by foldMemoryOperandImpl has mayLoad / mayLoadorStore set
like in the previous patch ?

Thanks,
Prathamesh