LGTM

In D71122#1774935, @masakiarai wrote:

Thank you very much for your quick review.
I don't have commit access.
Could you commit this patch?

Committed:
https://github.com/llvm/llvm-project/commit/caabb713ea157f8c449c8d3eb00410bbef734a22

Thanks for catching that!

In D70213#1756873, @jmolloy wrote:

LGTM, sorry for the delay.

@jmolloy, could you take one more look when you get a chance?

Committed with 03da6e8c00de2320e6b9dacba8f7850faceae319

Looks good to me.

Awesome cleanup!

In D68205#1689574, @jmolloy wrote:

Hi Thomas,

I made an example to show how this is handled.

%11:intregs = S2_addasl_rrri %7, %6, 1, post-instr-symbol <mcsymbol Stage-0_Cycle-0>
%12:intregs = L2_loadruh_io %11, -4, post-instr-symbol <mcsymbol Stage-1_Cycle-0> :: (load 2 from %ir.cgep2, !tbaa !0)
%5:intregs = S2_storerh_pi %6, -2, %12, post-instr-symbol <mcsymbol Stage-2_Cycle-0> :: (store 2 into %ir.lsr.iv, !tbaa !0)
ENDLOOP0 %bb.3, implicit-def $pc, implicit-def $lc0, implicit $sa0, implicit $lc0

We generate this code, annotated:

 <... prolog, boring ...>
 
bb.3.b2 (address-taken):  // Kernel.
  successors: %bb.3(0x7c000000), %bb.10(0x04000000)
 
  %15:intregs = PHI %25, %bb.6, %11, %bb.3
  %17:intregs = PHI %26, %bb.6, %12, %bb.3
  %11:intregs = S2_addasl_rrri %7, %6, 1, post-instr-symbol <mcsymbol Stage-0_Cycle-0>
  %12:intregs = L2_loadruh_io %15, -4, post-instr-symbol <mcsymbol Stage-1_Cycle-0> :: (load 2 from %ir.cgep2, !tbaa !0)
  dead %5:intregs = S2_storerh_pi %6, -2, %17, post-instr-symbol <mcsymbol Stage-2_Cycle-0> :: (store 2 into %ir.lsr.iv, !tbaa !0)
  ENDLOOP0 %bb.3, implicit-def $pc, implicit-def $lc0, implicit $sa0, implicit $lc0
  J2_jump %bb.10, implicit-def $pc
 
bb.10.b2:  // Epilog 0, runs stage 2
  successors: %bb.9(0x80000000)
 
  %40:intregs = PHI %11, %bb.3, %25, %bb.6
  %41:intregs = PHI %12, %bb.3, %26, %bb.6
  dead %44:intregs = S2_storerh_pi %6, -2, %41, post-instr-symbol <mcsymbol Stage-2_Cycle-0> :: (store 2 into %ir.lsr.iv, !tbaa !0)
  J2_jump %bb.9, implicit-def $pc
 
bb.9.b2:  // Start of Epilog 1, runs stage 1
  successors: %bb.8(0x80000000)
 
  %35:intregs = PHI %40, %bb.10, %20, %bb.5
  %38:intregs = L2_loadruh_io %35, -4, post-instr-symbol <mcsymbol Stage-1_Cycle-0> :: (load 2 from %ir.cgep2, !tbaa !0)
  J2_jump %bb.8, implicit-def $pc
 
bb.8.b2:  // Next stage of Epilog 1, runs stage 2
  successors: %bb.7(0x80000000)
 
  dead %34:intregs = S2_storerh_pi %6, -2, %38, post-instr-symbol <mcsymbol Stage-2_Cycle-0> :: (store 2 into %ir.lsr.iv, !tbaa !0)
  J2_jump %bb.7, implicit-def $pc

The key is that though E1 runs stages {1,2}, we *don't* create a block with both stages {1,2} enabled. This would cause the invalid code issue you mentioned. Instead, we expand this into *two* blocks. The first performs stage 1, the second stage 2 which consumes its input from stage 1.

That means we do generate superfluous epilog blocks, but these get merged by the control flow optimizer later.

That said, I'm not guaranteeing there are no bugs here. Perhaps the testcase you're thinking of distills to something more complex than my testcase?

Cheers,

James

In D67121#1663409, @arsenm wrote:

In D67121#1663397, @arsenm wrote:

In D67121#1656980, @jmolloy wrote:

On the other hand, I don't really want to spend much time thinking about how to make SelectionDAG better at this point.

I agree, that's kind of my worry with opening this pandora's box. Changing hooks in SDAG and maintaining performance parity is very hard, and we have a longer term solution in Global Isel.

Given that, and given the NFC-ness of this patch, are you happy for it to go in?

I'm not happy about it but not too opposed. I'd like someone else's opinion

I would somewhat prefer we split the current function into something like memoryAccessIsLegalForAlignment that has the current behavior, and this would be a separate function defaulting to calling it (Yes, I know this makes the problem worse)

I added a new function allowsMemoryAccessForAlignment to handle the behavior of the previous allowsMemoryAccess function. I replaced the calls every except for DAGCombine as my understanding is that this place really mean to check that the memory access is legal and won't require legalization to expensive code sequence.

Your summary says that the extended tracking is disable by default on hexagon. Which extra tracking is that? I don't see any part disabled for Hexagon?

In D67121#1656161, @arsenm wrote:

It seems to me like this is a workaround for the quite inadequate system for defining legality for a load/store. I think allowsMemoryAccess is poorly named; It exists to just check if the type is aligned before calling allowsMisalignedMemoryAccesses. Your problem sounds more like we need a better system for checking if a store/trunc store is legal

Looks good to me

This is great, will be very helpful.

Looks good to me.

I figured the functionality can be tested with Hexagon backend, so I added a test in Hexagon target to test the predication.

In D66190#1628864, @lebedev.ri wrote:

One more question - i can't recall if there's precedent for bundling more than one pass
into the same source file, that does not look ideal.

In D66190#1628712, @lebedev.ri wrote:

Tests missing; please upload all patches with full context (-U99999)

In D65424#1610454, @kparzysz wrote:

This seems to hang when compiling HexagonInstrInfo.cpp.