llvm dev - Sep 2020 - 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
> 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 --------------
An HTML attachment was scrubbed...
URL:
<http://lists.llvm.org/pipermail/llvm-dev/attachments/20200909/05796e8a/attachment.html>

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>