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

[Quentin Colombet via llvm-dev]

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>