♟ reames

In D57601#1404320, @jyknight wrote:

In D57601#1397317, @reames wrote:

Question for Reviewers: Going through uses of isVolatile in backend code, I see suspicious bits in Lania, ARM, and Hexagon. What's the best way to handle this? I can easily apply the workaround I mention for out of tree backends, or I could make them equally conservative for isAtomic. The other in tree backends appear to be fine, though I'd welcome review from folks knowledgeable of non-x86 backends.

I think adding the isAtomic check to the appropriate locations that are checking isVolatile makes the most sense, since it's easier to see and undo in the future, if that's desired.

D58490 is posted for review.

I'm fine with this landing, since the patch is an obvious improvement. However, I have to admit I don't understand the interaction between LCSSA and SCEV here. Why does SCEV need to care that a phi might be a LCSSA phi? They seem orthogonal. Maybe *some particular caller* might need to care, but that seems like a different issue?

In D57601#1401075, @asb wrote:

It seems there's a clear plan on the path forwards - is it worth writing it up as a short RFC on the mailing list, particular given the potential for subtle issues for out-of-tree backends?

I was planning on sending a note after it lands. Happy to do so now if folks things it's warranted.

Does the existing code successfully hoist the widenable_condition call itself out of the loop? If not, splitting out specifically that part of the transform would be well called for and necessary for unswitch to make progress. If it does, then the patch as written includes redundant handling which should be removed.

JFYI, I disagree w/the sentiment expressed that we shouldn't do obvious memory optimizations in InstCombine. InstCombine does local peephole optimizations *including memory optimizations*. Even if I accept the stated goal that it *shouldn't*, today it *does* and there's no reason to reject this patch.

[GVN] Small tweaks to comments, style, and missed vector handling

[GVN] Fix last crasher w/non-integral pointers

[GVN] Fix a crash bug w/non-integral pointers and memtransfers

[GVN] Fix a non-integral pointer bug w/vector types

[GVN] Fix a crash bug around non-integral pointers

[Test] Autogenerate existing tests before adding more

This looks to be a form of unswitching. Can you explain why this needs to be done in LICM rather than in unswitching?

[InstCombine] Address a couple stylistic issues pointed out by reviewer [NFC]

[InstCombine] Convert atomicrmws to xchg or store where legal

[Tests] Demonstrate more missing atomicrmw transforms

[InstCombine] Add todos for possible atomicrmw transforms

Canonicalize all integer "idempotent" atomicrmw ops

Address review comments

Teach instcombine about remaining "idempotent" atomicrmw types

[Tests] Add tests for all idemptotent atomicrmws

Question for Reviewers: Going through uses of isVolatile in backend code, I see suspicious bits in Lania, ARM, and Hexagon. What's the best way to handle this? I can easily apply the workaround I mention for out of tree backends, or I could make them equally conservative for isAtomic. The other in tree backends appear to be fine, though I'd welcome review from folks knowledgeable of non-x86 backends.

No longer WIP, ready for real review.

[SelectionDAG] Inline a single use helper function, and remove last non-MMO…

[SelectionDAG] Kill last uses of getAtomic w/o a MMO operand [NFC]

[Tests] More unordered atomic lowering tests

[Tests] RMW folding tests w/unordered atomic operations

[Tests] Add a bunch of tests for load folding w/unordered atomics

[Tests] First batch of cornercase tests for unordered atomics

[Tests] Auto update a test

[Tests] Rename some test files for consistency

[Tests] A few more live-in deopt lowering tests

[InlineSpiller] Fix a crash due to lack of forward progress from remat (try 2)

[Statepoint Lowering] Update misleading comments about chains

[Test] Use autogenerated checks for more statepoint tests

[Tests] Fill out a few tests around gc relocation uniquing

[Test] Autogenerate a statepoint test and actual show the reload

Be conservative about unordered accesses for the moment

LGTM w/two required follow ups. Can be before or after landing.

[AliasSetTracker] Minor style tweak to avoid a variable w/two distinct live…

[Test] Add codegen tests for unordered and monotonic integer operations

Marking just remove from review queue since discussion appears to have stalled.

Sorry for just getting around to reviewing this now. Should be a bit more response now.

reames (Philip Reames)
User

Projects

User Details

Recent Activity
View All

Thu, Feb 21

Wed, Feb 20

Tue, Feb 19

Fri, Feb 15

Thu, Feb 14

Wed, Feb 13

Tue, Feb 12

Mon, Feb 11

Fri, Feb 8

Wed, Feb 6

Tue, Feb 5

Mon, Feb 4

reames (Philip Reames)User