Incidentally I think it would be simpler if AMDGPU::getNumFlatOffsetBits did not take a "signed" argument, always returned the total number of bits in the field, and it was the caller's responsibility to worry about whether negative offsets are allowed or not.

Add a DBG_VALUE test case.

c28f79a0e3352a972026f6dc896d13f81835b468

Use eraseFromParentAndMarkDBGValuesForRemoval.

I have no objection to this as a first step in the right direction, but I think it should have no effect at all at the moment. The schedmodel already knows that we can only issue one instruction per cycle, and this patch does not affect that. It only becomes interesting when you model the fact that issuing a trans instruction consumes the trans ALU for (say) 4 cycles, so that on the following 3 cycles you could issue a normal VALU instruction but *not* another trans instruction.

... but adding @rampitec for awareness.

Seems fine to me. There is not much consistency in the naming of the SIInstrFlags flags.

Should I do the same for is_flat_scratch/global?

This removes the "Folded" set that @hliao added in D93174.

D76500 has landed now, so maybe abandon this review?

In D99743#2674198, @sebastian-ne wrote:

Use SIInstrFlags instead of new enum.

The '\n' is redundant since MI print includes it already

I think it was also causing this failure in my Release+Asserts build:

/home/jayfoad2/git/llvm-project/llvm/test/Transforms/WholeProgramDevirt/branch-funnel.ll:150:16: error: CHECK-LABEL: expected string not found in input
; CHECK-LABEL: define internal void @branch_funnel(i8*
               ^
<stdin>:81:16: note: scanning from here
define i32 @fn3(i8* nocapture readonly %obj) local_unnamed_addr #2 {
               ^
<stdin>:95:18: note: possible intended match here
define hidden void @__typeid_typeid1_0_branch_funnel(i8* nest %0, ...) local_unnamed_addr #6 {
                 ^
...
Failed Tests (1):
  LLVM :: Transforms/WholeProgramDevirt/branch-funnel.ll

Is there some overlap with SIInstrFlags::FLAT, SIInstrFlags::IsFlatScratch, SIInstrFlags::IsFlatGlobal? It seems like your enum is encoding the same thing in a slightly different way. See also the is*FLAT* helper methods in SIInstrInfo.

Rebase.

LGTM.

In D99352#2653253, @rampitec wrote:

@foad can we run it trough gfx perf suite? With exception of one ds_write_b64 store it seems reasonable to me. Of course these splits to b8 would be unfortunate but I assume it shall not happen in real life too often and the data is usually better aligned.

Looks good to me apart from the G_FCONSTANT issues.

LGTM, thanks!

Abandoning. It was just an experiment. There is no real use case for it.

Looks fine to me, thanks.

Seems obviously fine to me.

Please make the commit message a bit more descriptive than just "update".

Looks fine to me modulo the naming nit.

The code change looks fine to me. The added tests aren't actually testing any behaviour that has changed, are they? Is there any reasonable way to add tests for the unable-to-legalize case?

In general I am uncomfortable about generating code that does not work (i.e. expanding spills the way we do when exec might be 0) and then running yet another pass for correctness to fix it up later. Is there a way this can be made correct by default, and if necessary run an extra pass that optimizes it for efficiency?

Adding @Joe_Nash since he made some changes related to _e64 suffixes quite recently.