Page MenuHomePhabricator

GlobalISel: Allow forming atomic/volatile G_ZEXTLOAD
Needs ReviewPublic

Authored by arsenm on Apr 11 2022, 6:35 AM.

Details

Summary

SelectionDAG has a target hook, getExtendForAtomicOps, which it uses
in the computeKnownBits implementation for ATOMIC_LOAD. This is pretty
ugly (as is having a separate load opcode for atomics), so instead
allow making use of atomic zextload. Enable this for AArch64 since the
DAG path defaults in to the zext behavior.

The tablegen changes are pretty ugly, but partially helps migrate
SelectionDAG from using ISD::ATOMIC_LOAD to regular ISD::LOAD with
atomic memory operands. For now the DAG emitter will emit matchers for
patterns which the DAG will not produce.

I'm still a bit confused by the intent of the isLoad/isStore/isAtomic
bits. The DAG implementation rejects trying to use any of these in
combination. For now I've opted to make the isLoad checks also check
isAtomic, although I think having isLoad and isAtomic set on these
makes most sense.

Diff Detail

Event Timeline

arsenm created this revision.Apr 11 2022, 6:35 AM
arsenm requested review of this revision.Apr 11 2022, 6:35 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 11 2022, 6:35 AM
Herald added a subscriber: wdng. · View Herald Transcript
reames added a comment.May 3 2022, 1:16 PM

I don't feel comfortable reviewing this given lack of experience with global isel and aarch64, but I wanted to add a bit of context which might help explain some of the confusion about the code structure.

A while back I had started a transition for x86 from using a separate ATOMIC_LOAD opcode to using a generic load with appropriate atomic bits set. Unfortunately, I hit an issue in legalization that I got stuck on, and then got distracted and pulled away. As such, I suspect the code is in a somewhat misleading form. I vaguely remember the legalization issue as having something to do with atomic floating point loads, but I'm really not sure about that.

At a high level, I do very much think having atomic loads simply be normal loads with some atomic flags makes a lot of sense.

At a high level, I do very much think having atomic loads simply be normal loads with some atomic flags makes a lot of sense.

This is already the case for GlobalISel. I'm just extending to extending loads, rather than repeating the special case computeKnownBits hack the DAG uses (plus another poorly conceived target hook to report the atomic high bit behavior)

paquette added a subscriber: t.p.northover.

@t.p.northover What do you think about this?