[llvm-dev] Change prototype for TargetInstrInfo::foldMemoryOperandImpl

[Prathamesh Kulkarni via llvm-dev]

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

On Wed, 9 Sep 2020 at 22:30, Quentin Colombet <qcolombet at apple.com> wrote:
>
> 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
>
> On Sep 7, 2020, at 2:17 AM, Prathamesh Kulkarni via llvm-dev <llvm-dev at lists.llvm.org> wrote:
>
> 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
> _______________________________________________
> LLVM Developers mailing list
> llvm-dev at lists.llvm.org
> https://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-dev
>
>
-------------- next part --------------
A non-text attachment was scrubbed...
Name: foldmemoperand.diff
Type: application/octet-stream
Size: 925 bytes
Desc: not available
URL: <http://lists.llvm.org/pipermail/llvm-dev/attachments/20200910/59034fca/attachment.obj>