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

[Prathamesh Kulkarni via llvm-dev]

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

On Thu, 10 Sep 2020 at 21:43, Quentin Colombet <qcolombet at apple.com> wrote:
>
> Would it make sense to attach mayLoad to ARM::tBL instead?
>
> > On Sep 10, 2020, at 4:16 AM, Prathamesh Kulkarni <prathamesh.kulkarni at linaro.org> wrote:
> >
> > 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
> >>
> >>
> > <foldmemoperand.diff>
>