[PATCH] D104293: [AMDGPU] Set more flags on Real instructions

[Patrick Holland via Phabricator via llvm-commits]

holland11 added a comment.

> I've pushed some patches which should fix all of this: rG323b3e645dd3 <https://reviews.llvm.org/rG323b3e645dd340e7ffb86b06d33d071fdf4fb432>, rG24ffc343f9da <https://reviews.llvm.org/rG24ffc343f9da5bcfa007e5f9b52a3a023982ac89>, rG7f3ac6714a56 <https://reviews.llvm.org/rG7f3ac6714a561553500cbd24331a8dc7f2375964>

Awesome, thank you very much for this. I will update the AMDGPUCustomBehaviour implementation and then post it for review again.

> I don't think you can copy the TSFlags all at once.

Out of curiosity, why doesn't the `DS_Real` class have the `let TSFlags = ps.TSFlags` line that all of the other classes in this patch have (all of the classes that inherit from `InstSI` anyways: `SM_Real`, `FLAT_Real`, `MUBUF_Real`, and `MTBUF_Real`).

I'm newer to tablegen and it's a really quirky 'language' so I may be misunderstanding something, but it seems like it is possible to copy all of the TSFlags at once and most classes were already doing. (Many of them were still missing their mayLoad and mayStore flags before this patch though.)

I just did a quick search for all classes that inherit from `InstSI` (and have a `psuedo` instruction as one of the input operands) to find which ones **don't** have the `let TSFlags = ps.TSFlags` line.

`EXPInstructions.td`:

  class EXPCommon<bit done, string asm = ""> : InstSI<
    (outs),
    (ins exp_tgt:$tgt,
         ExpSrc0:$src0, ExpSrc1:$src1, ExpSrc2:$src2, ExpSrc3:$src3,
         exp_vm:$vm, exp_compr:$compr, i32imm:$en),
    asm> {
    let EXP = 1;
    let EXP_CNT = 1;
    let mayLoad = done;
    let mayStore = 1;
    let UseNamedOperandTable = 1;
    let Uses = [EXEC];
    let SchedRW = [WriteExport];
    let DisableWQM = 1;
  }
  
  class EXP_Real<bit done, string pseudo, int subtarget>
    : EXPCommon<done, "exp$tgt $src0, $src1, $src2, $src3"#!if(done, " done", "")
                      #"$compr$vm">,
      SIMCInstr <pseudo, subtarget> {
    let AsmMatchConverter = "cvtExp";
  }

`SOPInstructions.td`:

  class SOP1_Real<bits<8> op, SOP1_Pseudo ps, string real_name = ps.Mnemonic> :
    InstSI <ps.OutOperandList, ps.InOperandList,
            real_name # " " # ps.AsmOperands, []>,
    Enc32 {
  
    let isPseudo = 0;
    let isCodeGenOnly = 0;
    let Size = 4;
  
    // copy relevant pseudo op flags
    let SubtargetPredicate = ps.SubtargetPredicate;
    let AsmMatchConverter  = ps.AsmMatchConverter;
    let SchedRW            = ps.SchedRW;
  
    // encoding
    bits<7> sdst;
    bits<8> src0;
  
    let Inst{7-0} = !if(ps.has_src0, src0, ?);
    let Inst{15-8} = op;
    let Inst{22-16} = !if(ps.has_sdst, sdst, ?);
    let Inst{31-23} = 0x17d; //encoding;
  }



  class SOPK_Real<bits<5> op, SOPK_Pseudo ps> :
    InstSI <ps.OutOperandList, ps.InOperandList,
            ps.Mnemonic # " " # ps.AsmOperands, []> {
    let isPseudo = 0;
    let isCodeGenOnly = 0;
  
    // copy relevant pseudo op flags
    let SubtargetPredicate = ps.SubtargetPredicate;
    let AsmMatchConverter  = ps.AsmMatchConverter;
    let DisableEncoding    = ps.DisableEncoding;
    let Constraints        = ps.Constraints;
    let SchedRW            = ps.SchedRW;
  
    // encoding
    bits<7>  sdst;
    bits<16> simm16;
    bits<32> imm;
  }

`DSInstructions.td`:

  class DS_Real <DS_Pseudo ds> :
    InstSI <ds.OutOperandList, ds.InOperandList, ds.Mnemonic # ds.AsmOperands, []>,
    Enc64 {
  
    let isPseudo = 0;
    let isCodeGenOnly = 0;
    let DS = 1;
    let UseNamedOperandTable = 1;
  
    // copy relevant pseudo op flags
    let SubtargetPredicate = ds.SubtargetPredicate;
    let OtherPredicates    = ds.OtherPredicates;
    let AsmMatchConverter  = ds.AsmMatchConverter;
    let SchedRW            = ds.SchedRW;
  
    // encoding fields
    bits<10> vdst;
    bits<1> gds;
    bits<8> addr;
    bits<10> data0;
    bits<10> data1;
    bits<8> offset0;
    bits<8> offset1;
  
    bits<16> offset;
    let offset0 = !if(ds.has_offset, offset{7-0}, ?);
    let offset1 = !if(ds.has_offset, offset{15-8}, ?);
  
    bits<1> acc = !if(ds.has_vdst, vdst{9},
                      !if(!or(ds.has_data0, ds.has_gws_data0), data0{9}, 0));
  }

The majority of classes that inherit from `InstSI` **and** have a `Psuedo` instruction as an input operand do have `let TSFlags = ps.TSFlags`, but these ones are the exceptions.

You'd know better than I, but maybe adding that line to the above classes (combined with how you've already added the mayLoad and mayStore flags in this patch) would do the job?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D104293/new/

https://reviews.llvm.org/D104293