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

[Quentin Colombet via llvm-dev]

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>