Pierre-vh added a comment.

I think that if this is a new property of the GFX940/941 targets, and turning 
it off shouldn't be possible, we shouldn't even bother with a feature and just 
set a bool in the ST for those targets



================
Comment at: llvm/lib/Target/AMDGPU/SIMemoryLegalizer.cpp:524
+         SIAtomicAddrSpace::NONE)
+      return enableSC0Bit(MI) | enableSC1Bit(MI);
+    return false;
----------------
kzhuravl wrote:
> jmmartinez wrote:
> > NIT: Is the use of the bitwise or " | " intended? I'd use the logical or " 
> > || " instead.
> It is intentional, we need both SC0 and SC1 bits set. If I switch this to || 
> it will short circuit and not invoke enableSC1Bit.
IMHO then it needs a comment to explain that it's intentional, otherwise some 
innocent maintainer in the future could think it's a typo and change it


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D149986

_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
  • [PATCH] D149... Matt Arsenault via Phabricator via cfe-commits
    • [PATCH]... Konstantin Zhuravlyov via Phabricator via cfe-commits
    • [PATCH]... Juan Manuel Martinez CaamaƱo via Phabricator via cfe-commits
    • [PATCH]... Konstantin Zhuravlyov via Phabricator via cfe-commits
    • [PATCH]... Pierre van Houtryve via Phabricator via cfe-commits
    • [PATCH]... Konstantin Zhuravlyov via Phabricator via cfe-commits
    • [PATCH]... Konstantin Zhuravlyov via Phabricator via cfe-commits
    • [PATCH]... Scott Linder via Phabricator via cfe-commits
    • [PATCH]... Matt Arsenault via Phabricator via cfe-commits

Reply via email to