[PATCH] D59547: [X86] Remove X86 specific dag nodes for RDTSC/RDTSCP/RDPMC. NFCI

Andrea Di Biagio via Phabricator via llvm-commits llvm-commits at lists.llvm.org
Tue Mar 19 14:28:51 PDT 2019


andreadb marked an inline comment as done.
andreadb added inline comments.


================
Comment at: lib/Target/X86/X86ISelLowering.cpp:23090
     SmallVector<SDValue, 2> Results;
-    getReadPerformanceCounter(Op.getNode(), dl, DAG, Subtarget, Results);
+    // Handles the lowering of builtin intrinsics that read performance monitor
+    // counters (x86_rdpmc).
----------------
craig.topper wrote:
> andreadb wrote:
> > andreadb wrote:
> > > craig.topper wrote:
> > > > I can't decide if these should really be in the X86IntrinsicsInfo.h table. We're not using the Opc0 field. So we're treating them as one offs. Which makes me wonder if they shouldn't just be in the switch that is used when the table lookup fails.
> > > > 
> > > > Or maybe we should merge RDPMC and XGETBV using the Opc0 field?
> > > I think it is a good idea to just merge these two cases and simply write:
> > > ```
> > > expandIntrinsicWChainHelper(Op.getNode(), dl, DAG, IntrData->Opc0, X86::ECX,
> > >                                 Subtarget, Results);
> > > ```
> > Actually, we could also merge `case RDTSC` along RDPMC and XGETBV .
> > In the case of RDTSC, function `getReadTimeStampCounter()` simply delegates to `expandIntrinsicWChainHelper()`.
> > 
> > Would it be okay if for now I merge these three cases togheter?
> Sounds good.
Ah... nevermind.
I didn't realize that `RDTSC` is used as `IntrData->Type` for both intrinsic `rdtsc` and `rdtscp`.
That means, It is not safe to also merge `case RDTSC` with RDPMC and XGETBV, because `rdtscp` requires a special treatment.

For now, I am going to just merge RDPMC and XGETBV together.


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

https://reviews.llvm.org/D59547





More information about the llvm-commits mailing list