[PATCH] D51969: [AMDGPU] Add an AMDGPU specific atomic optimizer.

[Nicolai Hähnle via Phabricator via llvm-commits]

nhaehnle added inline comments.


================
Comment at: lib/Target/AMDGPU/AMDGPUAtomicOptimizer.cpp:243-244
+  Value *const ExtractHi = B.CreateExtractElement(BitCast, B.getInt32(1));
+  Function *const MbcntLoIntrinsic =
+      Intrinsic::getDeclaration(I.getModule(), Intrinsic::amdgcn_mbcnt_lo);
+  Function *const MbcntHiIntrinsic =
----------------
sheredom wrote:
> nhaehnle wrote:
> > sheredom wrote:
> > > arsenm wrote:
> > > > CreateIntrinsic
> > > I can't use the IRBuilder's CreateIntrinsic because it will then try and mangle the amdgcn_mbcnt_lo like llvm.amdgcn.mbcnt.lo.i32, whereas we expect the name to be sans-type-suffix.
> > Ugh, this is a bug in CreateIntrinsic. It does:
> > ```
> >   Function *Fn = Intrinsic::getDeclaration(M, ID, { Args.front()->getType() });
> > ```
> > ... when in this particular case, the type vector should be empty.
> > 
> > There aren't that many users of CreateIntrinsic -- can you just fix CreateIntrinsic so that it takes an `ArrayRef<Type*>` like Intrinsic::getDeclaration? (As a separate preparatory patch obviously.)
> Just to be clear - do you want me to get the IRBuilder fix in first? Or is it fine to let this patch in as is, and then fixup during the IRBuilder change?
I think we should do it right the first time.


https://reviews.llvm.org/D51969