[PATCH] D48832: [ARM] ARMCodeGenPrepare backend pass

[Sam Parker via Phabricator via llvm-commits]

samparker added a comment.

@efriedma what was the extra instruction that you're generating in your example? I'm seeing code that looks okay to me:

  ldrh	r0, [r0]
  movw	r2, #65533
  cmp	r0, r2



================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:49
+static cl::opt<bool>
+DisableDSP("arm-disable-scalar-dsp", cl::Hidden, cl::init(false),
+          cl::desc("Don't use DSP instructions for scalar operations"));
----------------
SjoerdMeijer wrote:
> Can you comment what this exactly does? Looks like that deep down in the logic that checks for candidares, in ARMCodeGenPrepare::isNarrowInstSupported(Instruction *I), this prevents the transformation from triggering. So checking DisableDSP can be done a lot earlier, or perhaps you want to put a TODO somewhere if you plan on supporting this later? Also, this one does not seem to be covered by tests (arm-disable-scalar-dsp-imms is covered).
It's to control the generation of the DSP instructions which, as Eli points out, could be unsafe. I only want to check at that point because the transformation could happen without the need for those intrinsics.

I'll add some tests, and I'm going to change the default of this to 'true' to stay on the safe side.


================
Comment at: lib/Target/ARM/ARMCodeGenPrepare.cpp:280
+  case Instruction::Add:
+    return TypeSize == 16 ? Intrinsic::arm_uadd16 :
+      Intrinsic::arm_uadd8;
----------------
efriedma wrote:
> Is it safe to generate calls to uadd?  It writes to the GE bits, and user code could potentially read them.
Currently intrinsics are the only way to read and write the GE flag, but no it doesn't mean its safe. Is there a way to query the function/module for the intrinsics used? If there's no sel intrinsic being used, we are okay to use these instructions. Either way, I'll change the default to disable the generation of these instructions.


https://reviews.llvm.org/D48832