[PATCH] D86878: [AMDGPU] Fix a miscompile with S_ADD/S_SUB

[Piotr Sobczak via Phabricator via llvm-commits]

piotr added inline comments.


================
Comment at: llvm/lib/Target/AMDGPU/AMDGPUISelDAGToDAG.cpp:1056
+    unsigned CondOpc = CI->getOpcode();
+    if (CondOpc == ISD::AND || CondOpc == ISD::OR || CondOpc == ISD::XOR) {
+      auto ST = static_cast<const GCNSubtarget *>(Subtarget);
----------------
arsenm wrote:
> piotr wrote:
> > arsenm wrote:
> > > I don't think it will end up mattering with the operations legal for i1 (although maybe trunc is also a problem), but I think it would be somewhat safer to list valid boolean sources instead
> > Will do, but what do you mean exactly by "valid boolean sources" here?
> I mean like setcc + class intrinsics, other things that will select to VOPC outputs
I looked at this and I am not convinced we need to handle setcc here (added TODO). And fp_class would be mapped to VALU, so no need to handle this here either. 


================
Comment at: llvm/test/CodeGen/AMDGPU/combine-add-zext-xor.ll:16
+
+define void @combine_add_zext_xor() {
+.entry:
----------------
arsenm wrote:
> piotr wrote:
> > arsenm wrote:
> > > Should be able to reduce this more
> > This already comes from bugpoint, but I will try to reduce the test a bit more.
> Bugpoint isn't that smart and you can usually massage the IR a bit to help it make more progress
Simplified test (also added "-start-before=amdgpu-isel -stop-after=amdgpu-isel") and added 5 more cases.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D86878